* [PATCHv1 0/6] Support for GE B1x5v2
@ 2021-02-22 17:12 Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 1/6] rtc: m41t80: add support for protected clock Sebastian Reichel
` (5 more replies)
0 siblings, 6 replies; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
Hi,
This series adds support for another General Electric patient
monitor series (similar to existing Bx50v3), which is based on
i.MX6DL using Congatec's QMX6 module.
The module uses an I2C RTC to provide the i.MX6 32768 Hz clock,
so it's important to keep it enabled. Not doing so results in
incorrect timings of watchdog and i.MX6 RTC. The bootloader
enables the watchdog, so disabling the clock results in system
reboot. [0]
The second patch is required for B155v2, which uses a 1366x768
G156XTN01 panel. The 1366 width is not supported by the display
pipeline and result in boot hanging without the patch. [1]
Patches 3+4 are updating DT bindings for the new board compatible
values.
Patch 5 adds missing sst25vf032b to spi-nor bindings. Checkpatch
still complains, since the binding lists all chips without vendor
prefix. This probably should be fixed when the files is moved to
YAML, but is non-trivial since those chips are manufactured by
multiple vendors. E.g. sst25vf032b can be sourced from at least
sst and microchip.
Finally patch 6 adds the board files.
Thanks,
[0] There has been a discussion for the problem on the mailinglists
last year. The discussion died off, when I told people their ideas
don't work. I hope using protected-clocks is fine for this usecase.
https://lore.kernel.org/linux-clk/20191108170135.9053-1-sebastian.reichel@collabora.com/
[1] I've sent this before as a separate patch in September, but
nobody seemed to care. This adds full context for the problem.
https://lore.kernel.org/dri-devel/20200910162831.321556-1-sebastian.reichel@collabora.com/
-- Sebastian
Sebastian Reichel (6):
rtc: m41t80: add support for protected clock
drm/imx: Add 8 pixel alignment fix
dt-bindings: vendor-prefixes: add congatec
dt-bindings: arm: fsl: add GE B1x5pv2 boards
dt-bindings: mtd: jedec,spi-nor: add sst25vf032b
ARM: dts: imx6: Add GE B1x5v2
.../devicetree/bindings/arm/fsl.yaml | 11 +
.../devicetree/bindings/mtd/jedec,spi-nor.txt | 1 +
.../devicetree/bindings/rtc/rtc-m41t80.txt | 1 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
arch/arm/boot/dts/Makefile | 5 +
arch/arm/boot/dts/imx6dl-b105pv2.dts | 35 +
arch/arm/boot/dts/imx6dl-b105v2.dts | 35 +
arch/arm/boot/dts/imx6dl-b125pv2.dts | 33 +
arch/arm/boot/dts/imx6dl-b125v2.dts | 33 +
arch/arm/boot/dts/imx6dl-b155v2.dts | 36 +
arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi | 434 ++++++++++++
arch/arm/boot/dts/imx6dl-b1x5v2.dtsi | 61 ++
arch/arm/boot/dts/imx6dl-qmx6.dtsi | 623 ++++++++++++++++++
drivers/gpu/drm/imx/imx-drm-core.c | 19 +-
drivers/gpu/drm/imx/imx-ldb.c | 5 +
drivers/gpu/drm/imx/ipuv3-crtc.c | 11 +-
drivers/gpu/drm/imx/ipuv3-plane.c | 19 +-
drivers/gpu/ipu-v3/ipu-dc.c | 5 +
drivers/gpu/ipu-v3/ipu-di.c | 7 +
drivers/rtc/rtc-m41t80.c | 3 +
20 files changed, 1373 insertions(+), 6 deletions(-)
create mode 100644 arch/arm/boot/dts/imx6dl-b105pv2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b105v2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b125pv2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b125v2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b155v2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi
create mode 100644 arch/arm/boot/dts/imx6dl-b1x5v2.dtsi
create mode 100644 arch/arm/boot/dts/imx6dl-qmx6.dtsi
--
2.30.0
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
@ 2021-02-22 17:12 ` Sebastian Reichel
2021-02-22 21:20 ` Alexandre Belloni
2021-02-22 17:12 ` [PATCHv1 2/6] drm/imx: Add 8 pixel alignment fix Sebastian Reichel
` (4 subsequent siblings)
5 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
modules SQW clock output defaults to 32768 Hz. This behaviour is
used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
the clock is disabled and all i.MX6 functionality depending on
the 32 KHz clock has undefined behaviour. On systems using hardware
watchdog it seems to likely trigger a lot earlier than configured.
The proper solution would be to describe this dependency in DT,
but that will result in a deadlock. The kernel will see, that
i.MX6 system clock needs the RTC clock and do probe deferral.
But the i.MX6 I2C module never becomes usable without the i.MX6
CKIL clock and thus the RTC's clock will not be probed. So from
the kernel's perspective this is a chicken-and-egg problem.
Technically everything is fine by not touching anything, since
the RTC clock correctly enables the clock on reset (i.e. on
battery backup power loss) and also the bootloader enables it
in case a kernel without this support has been booted.
The 'protected-clocks' property is already in use for some clocks
that may not be touched because of firmware limitations and is
described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Documentation/devicetree/bindings/rtc/rtc-m41t80.txt | 1 +
drivers/rtc/rtc-m41t80.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
index c746cb221210..ea4bbf5c4282 100644
--- a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
+++ b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
@@ -19,6 +19,7 @@ Optional properties:
- interrupts: rtc alarm interrupt.
- clock-output-names: From common clock binding to override the default output
clock name
+- protected-clocks: Bool, if set operating system should not handle clock.
- wakeup-source: Enables wake up of host system on alarm
Example:
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 160dcf68e64e..3296583853a8 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -546,6 +546,9 @@ static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
struct clk_init_data init;
int ret;
+ if (of_property_read_bool(node, "protected-clocks"))
+ return 0;
+
/* First disable the clock */
ret = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_MON);
if (ret < 0)
--
2.30.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCHv1 2/6] drm/imx: Add 8 pixel alignment fix
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 1/6] rtc: m41t80: add support for protected clock Sebastian Reichel
@ 2021-02-22 17:12 ` Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 3/6] dt-bindings: vendor-prefixes: add congatec Sebastian Reichel
` (3 subsequent siblings)
5 siblings, 0 replies; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel,
Boris Brezillon
Some standard resolutions like 1366x768 do not work properly with
i.MX6 SoCs, since the horizontal resolution needs to be aligned
to 8 pixels (so 1360x768 or 1368x768 would work).
This patch allocates framebuffers allocated to 8 pixels. The extra
time required to send the extra pixels are removed from the blank
time. In order to expose the correct display size to userspace,
the stride is increased without increasing the width.
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
drivers/gpu/drm/imx/imx-drm-core.c | 19 ++++++++++++++++++-
drivers/gpu/drm/imx/imx-ldb.c | 5 +++++
drivers/gpu/drm/imx/ipuv3-crtc.c | 11 ++++++++++-
drivers/gpu/drm/imx/ipuv3-plane.c | 19 +++++++++++++++----
drivers/gpu/ipu-v3/ipu-dc.c | 5 +++++
drivers/gpu/ipu-v3/ipu-di.c | 7 +++++++
6 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index d1a9841adeed..1bcf740b7f4f 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -145,9 +145,26 @@ static const struct drm_ioctl_desc imx_drm_ioctls[] = {
/* none so far */
};
+static int imx_drm_dumb_create(struct drm_file *file_priv,
+ struct drm_device *drm,
+ struct drm_mode_create_dumb *args)
+{
+ u32 width = args->width;
+ int ret;
+
+ args->width = ALIGN(width, 8);
+
+ ret = drm_gem_cma_dumb_create(file_priv, drm, args);
+ if (ret)
+ return ret;
+
+ args->width = width;
+ return ret;
+}
+
static const struct drm_driver imx_drm_driver = {
.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
- DRM_GEM_CMA_DRIVER_OPS,
+ DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE(imx_drm_dumb_create),
.ioctls = imx_drm_ioctls,
.num_ioctls = ARRAY_SIZE(imx_drm_ioctls),
.fops = &imx_drm_driver_fops,
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 41e2978cb1eb..9c7710c719d7 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -257,6 +257,11 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder *encoder,
"%s: mode exceeds 85 MHz pixel clock\n", __func__);
}
+ if (!IS_ALIGNED(mode->hdisplay, 8)) {
+ dev_warn(ldb->dev,
+ "%s: hdisplay does not align to 8 byte\n", __func__);
+ }
+
if (dual) {
serial_clk = 3500UL * mode->clock;
imx_ldb_set_clock(ldb, mux, 0, serial_clk, di_clk);
diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 7ebd99ee3240..1ab970bcd52b 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -305,10 +305,19 @@ static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
sig_cfg.vsync_pin = imx_crtc_state->di_vsync_pin;
drm_display_mode_to_videomode(mode, &sig_cfg.mode);
+ if (!IS_ALIGNED(sig_cfg.mode.hactive, 8)) {
+ unsigned int new_hactive = ALIGN(sig_cfg.mode.hactive, 8);
+
+ dev_warn(ipu_crtc->dev, "8-pixel align hactive %d -> %d\n",
+ sig_cfg.mode.hactive, new_hactive);
+
+ sig_cfg.mode.hfront_porch = new_hactive - sig_cfg.mode.hactive;
+ sig_cfg.mode.hactive = new_hactive;
+ }
ipu_dc_init_sync(ipu_crtc->dc, ipu_crtc->di,
mode->flags & DRM_MODE_FLAG_INTERLACE,
- imx_crtc_state->bus_format, mode->hdisplay);
+ imx_crtc_state->bus_format, sig_cfg.mode.hactive);
ipu_di_init_sync_panel(ipu_crtc->di, &sig_cfg);
}
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 8a4235d9d9f1..5dd43e8c4d9e 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -29,6 +29,11 @@ to_ipu_plane_state(struct drm_plane_state *p)
return container_of(p, struct ipu_plane_state, base);
}
+static unsigned int ipu_src_rect_width(const struct drm_plane_state *state)
+{
+ return ALIGN(drm_rect_width(&state->src) >> 16, 8);
+}
+
static inline struct ipu_plane *to_ipu_plane(struct drm_plane *p)
{
return container_of(p, struct ipu_plane, base);
@@ -418,6 +423,12 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
if (old_fb && fb->pitches[0] != old_fb->pitches[0])
crtc_state->mode_changed = true;
+ if (ALIGN(fb->width, 8) * fb->format->cpp[0] >
+ fb->pitches[0] + fb->offsets[0]) {
+ dev_warn(dev, "pitch is not big enough for 8 pixels alignment");
+ return -EINVAL;
+ }
+
switch (fb->format->format) {
case DRM_FORMAT_YUV420:
case DRM_FORMAT_YVU420:
@@ -590,7 +601,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
if (ipu_state->use_pre) {
axi_id = ipu_chan_assign_axi_id(ipu_plane->dma);
ipu_prg_channel_configure(ipu_plane->ipu_ch, axi_id,
- drm_rect_width(&state->src) >> 16,
+ ipu_src_rect_width(state),
drm_rect_height(&state->src) >> 16,
fb->pitches[0], fb->format->format,
fb->modifier, &eba);
@@ -623,9 +634,9 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
break;
}
- ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(dst));
+ ipu_dmfc_config_wait4eot(ipu_plane->dmfc, ALIGN(drm_rect_width(dst), 8));
- width = drm_rect_width(&state->src) >> 16;
+ width = ipu_src_rect_width(state);
height = drm_rect_height(&state->src) >> 16;
info = drm_format_info(fb->format->format);
ipu_calculate_bursts(width, info->cpp[0], fb->pitches[0],
@@ -689,7 +700,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
ipu_cpmem_zero(ipu_plane->alpha_ch);
ipu_cpmem_set_resolution(ipu_plane->alpha_ch,
- drm_rect_width(&state->src) >> 16,
+ ipu_src_rect_width(state),
drm_rect_height(&state->src) >> 16);
ipu_cpmem_set_format_passthrough(ipu_plane->alpha_ch, 8);
ipu_cpmem_set_high_priority(ipu_plane->alpha_ch);
diff --git a/drivers/gpu/ipu-v3/ipu-dc.c b/drivers/gpu/ipu-v3/ipu-dc.c
index 34b4075a6a8e..ca96b235491a 100644
--- a/drivers/gpu/ipu-v3/ipu-dc.c
+++ b/drivers/gpu/ipu-v3/ipu-dc.c
@@ -167,6 +167,11 @@ int ipu_dc_init_sync(struct ipu_dc *dc, struct ipu_di *di, bool interlaced,
dc->di = ipu_di_get_num(di);
+ if (!IS_ALIGNED(width, 8)) {
+ dev_warn(priv->dev,
+ "%s: hactive does not align to 8 byte\n", __func__);
+ }
+
map = ipu_bus_format_to_map(bus_format);
/*
diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index b4a31d506fcc..f914e794a64e 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -510,6 +510,13 @@ int ipu_di_adjust_videomode(struct ipu_di *di, struct videomode *mode)
{
u32 diff;
+ if (!IS_ALIGNED(mode->hactive, 8) &&
+ mode->hfront_porch < ALIGN(mode->hactive, 8) - mode->hactive) {
+ dev_err(di->ipu->dev, "hactive %d is not aligned to 8 and front porch is too small to compensate\n",
+ mode->hactive);
+ return -EINVAL;
+ }
+
if (mode->vfront_porch >= 2)
return 0;
--
2.30.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCHv1 3/6] dt-bindings: vendor-prefixes: add congatec
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 1/6] rtc: m41t80: add support for protected clock Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 2/6] drm/imx: Add 8 pixel alignment fix Sebastian Reichel
@ 2021-02-22 17:12 ` Sebastian Reichel
2021-03-06 19:57 ` Rob Herring
2021-02-22 17:12 ` [PATCHv1 4/6] dt-bindings: arm: fsl: add GE B1x5pv2 boards Sebastian Reichel
` (2 subsequent siblings)
5 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
Document binding for congatec.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 041ae90b0d8f..a32db51df6c8 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -239,6 +239,8 @@ patternProperties:
description: Colorful GRP, Shenzhen Xueyushi Technology Ltd.
"^compulab,.*":
description: CompuLab Ltd.
+ "^congatec,.*":
+ description: congatec GmbH
"^coreriver,.*":
description: CORERIVER Semiconductor Co.,Ltd.
"^corpro,.*":
--
2.30.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCHv1 4/6] dt-bindings: arm: fsl: add GE B1x5pv2 boards
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
` (2 preceding siblings ...)
2021-02-22 17:12 ` [PATCHv1 3/6] dt-bindings: vendor-prefixes: add congatec Sebastian Reichel
@ 2021-02-22 17:12 ` Sebastian Reichel
2021-03-06 19:58 ` Rob Herring
2021-02-22 17:12 ` [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 6/6] ARM: dts: imx6: Add GE B1x5v2 Sebastian Reichel
5 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
Document the compatible for GE B1x5pv2 boards.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Documentation/devicetree/bindings/arm/fsl.yaml | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 34000f7fbe02..dd2b566314a0 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -393,6 +393,17 @@ properties:
- const: armadeus,imx6dl-apf6 # APF6 (Solo) SoM
- const: fsl,imx6dl
+ - description: i.MX6DL based congatec QMX6 Boards
+ items:
+ - enum:
+ - ge,imx6dl-b105v2 # General Electric B105v2
+ - ge,imx6dl-b105pv2 # General Electric B105Pv2
+ - ge,imx6dl-b125v2 # General Electric B125v2
+ - ge,imx6dl-b125pv2 # General Electric B125Pv2
+ - ge,imx6dl-b155v2 # General Electric B155v2
+ - const: congatec,qmx6
+ - const: fsl,imx6dl
+
- description: i.MX6DL based DFI FS700-M60-6DL Board
items:
- const: dfi,fs700-m60-6dl
--
2.30.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
` (3 preceding siblings ...)
2021-02-22 17:12 ` [PATCHv1 4/6] dt-bindings: arm: fsl: add GE B1x5pv2 boards Sebastian Reichel
@ 2021-02-22 17:12 ` Sebastian Reichel
2021-02-23 0:15 ` Rob Herring
2021-02-22 17:12 ` [PATCHv1 6/6] ARM: dts: imx6: Add GE B1x5v2 Sebastian Reichel
5 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
The binding is already used by the driver. Update documentation
accordingly.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
index f03be904d3c2..40e626e82ed0 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
@@ -31,6 +31,7 @@ Required properties:
s25sl12801
s25fl008k
s25fl064k
+ sst25vf032b
sst25vf040b
m25p40
m25p80
--
2.30.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCHv1 6/6] ARM: dts: imx6: Add GE B1x5v2
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
` (4 preceding siblings ...)
2021-02-22 17:12 ` [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b Sebastian Reichel
@ 2021-02-22 17:12 ` Sebastian Reichel
5 siblings, 0 replies; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-22 17:12 UTC (permalink / raw)
To: Sebastian Reichel, Philipp Zabel, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team
Cc: Rob Herring, Alexandre Belloni, Alessandro Zummo, David Airlie,
Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
This adds device tree files for the General Electric Healthcare
(GEHC) B1x5v2 series. All models make use of Congatec's QMX6 module,
which is described in its own device tree include, so that it can
also be used by other boards.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
arch/arm/boot/dts/Makefile | 5 +
arch/arm/boot/dts/imx6dl-b105pv2.dts | 35 ++
arch/arm/boot/dts/imx6dl-b105v2.dts | 35 ++
arch/arm/boot/dts/imx6dl-b125pv2.dts | 33 ++
arch/arm/boot/dts/imx6dl-b125v2.dts | 33 ++
arch/arm/boot/dts/imx6dl-b155v2.dts | 36 ++
arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi | 434 ++++++++++++++++++
arch/arm/boot/dts/imx6dl-b1x5v2.dtsi | 61 +++
arch/arm/boot/dts/imx6dl-qmx6.dtsi | 623 ++++++++++++++++++++++++++
9 files changed, 1295 insertions(+)
create mode 100644 arch/arm/boot/dts/imx6dl-b105pv2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b105v2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b125pv2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b125v2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b155v2.dts
create mode 100644 arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi
create mode 100644 arch/arm/boot/dts/imx6dl-b1x5v2.dtsi
create mode 100644 arch/arm/boot/dts/imx6dl-qmx6.dtsi
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 3d1ea0b25168..cd6bf51d27ec 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -513,6 +513,11 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
imx6q-dms-ba16.dtb \
imx6q-emcon-avari.dtb \
imx6q-evi.dtb \
+ imx6dl-b105v2.dtb \
+ imx6dl-b105pv2.dtb \
+ imx6dl-b125v2.dtb \
+ imx6dl-b125pv2.dtb \
+ imx6dl-b155v2.dtb \
imx6q-gk802.dtb \
imx6q-gw51xx.dtb \
imx6q-gw52xx.dtb \
diff --git a/arch/arm/boot/dts/imx6dl-b105pv2.dts b/arch/arm/boot/dts/imx6dl-b105pv2.dts
new file mode 100644
index 000000000000..0d5be2f9471f
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b105pv2.dts
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B105Pv2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+/dts-v1/;
+#include "imx6dl-b1x5pv2.dtsi"
+
+/ {
+ model = "General Electric B105Pv2";
+ compatible = "ge,imx6dl-b105pv2", "congatec,qmx6", "fsl,imx6dl";
+
+ panel {
+ compatible = "auo,g101evn010";
+ status = "okay";
+ };
+};
+
+&i2c3 {
+ touchscreen@41 {
+ reg = <0x41>;
+ compatible = "ilitek,ili251x";
+
+ pinctrl-names = "default";
+ pinctrl-0 =<&pinctrl_q7_gpio0>;
+ interrupt-parent = <&gpio5>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&tca6424a 21 GPIO_ACTIVE_LOW>;
+
+ touchscreen-size-x = <1280>;
+ touchscreen-size-y = <800>;
+ };
+};
diff --git a/arch/arm/boot/dts/imx6dl-b105v2.dts b/arch/arm/boot/dts/imx6dl-b105v2.dts
new file mode 100644
index 000000000000..72a085348304
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b105v2.dts
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B105v2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+/dts-v1/;
+#include "imx6dl-b1x5v2.dtsi"
+
+/ {
+ model = "General Electric B105v2";
+ compatible = "ge,imx6dl-b105v2", "congatec,qmx6", "fsl,imx6dl";
+
+ panel {
+ compatible = "auo,g101evn010";
+ status = "okay";
+ };
+};
+
+&i2c3 {
+ touchscreen@41 {
+ reg = <0x41>;
+ compatible = "ilitek,ili251x";
+
+ pinctrl-names = "default";
+ pinctrl-0 =<&pinctrl_q7_gpio0>;
+ interrupt-parent = <&gpio5>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&tca6424a 21 GPIO_ACTIVE_LOW>;
+
+ touchscreen-size-x = <1280>;
+ touchscreen-size-y = <800>;
+ };
+};
diff --git a/arch/arm/boot/dts/imx6dl-b125pv2.dts b/arch/arm/boot/dts/imx6dl-b125pv2.dts
new file mode 100644
index 000000000000..8fd6c8ed6750
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b125pv2.dts
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B125Pv2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+/dts-v1/;
+#include "imx6dl-b1x5pv2.dtsi"
+
+/ {
+ model = "General Electric B125Pv2";
+ compatible = "ge,imx6dl-b125pv2", "congatec,qmx6", "fsl,imx6dl";
+
+ panel {
+ compatible = "auo,g121ean01";
+ status = "okay";
+ };
+};
+
+&i2c3 {
+ touchscreen@2a {
+ reg = <0x2a>;
+ compatible = "eeti,exc80h60";
+
+ pinctrl-names = "default";
+ pinctrl-0 =<&pinctrl_q7_gpio0>;
+ interrupt-parent = <&gpio5>;
+ interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+
+ reset-gpios = <&tca6424a 21 GPIO_ACTIVE_HIGH>;
+ };
+};
diff --git a/arch/arm/boot/dts/imx6dl-b125v2.dts b/arch/arm/boot/dts/imx6dl-b125v2.dts
new file mode 100644
index 000000000000..eb26ffde9f72
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b125v2.dts
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B125v2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+/dts-v1/;
+#include "imx6dl-b1x5v2.dtsi"
+
+/ {
+ model = "General Electric B125v2";
+ compatible = "ge,imx6dl-b125v2", "congatec,qmx6", "fsl,imx6dl";
+
+ panel {
+ compatible = "auo,g121ean01";
+ status = "okay";
+ };
+};
+
+&i2c3 {
+ touchscreen@2a {
+ reg = <0x2a>;
+ compatible = "eeti,exc80h60";
+
+ pinctrl-names = "default";
+ pinctrl-0 =<&pinctrl_q7_gpio0>;
+ interrupt-parent = <&gpio5>;
+ interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+
+ reset-gpios = <&tca6424a 21 GPIO_ACTIVE_HIGH>;
+ };
+};
diff --git a/arch/arm/boot/dts/imx6dl-b155v2.dts b/arch/arm/boot/dts/imx6dl-b155v2.dts
new file mode 100644
index 000000000000..620cd6f9da82
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b155v2.dts
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B155v2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+/dts-v1/;
+#include "imx6dl-b1x5v2.dtsi"
+
+/ {
+ model = "General Electric B155v2";
+ compatible = "ge,imx6dl-b155v2", "congatec,qmx6", "fsl,imx6dl";
+
+ panel {
+ compatible = "auo,g156xtn01";
+ status = "okay";
+ };
+};
+
+&i2c3 {
+ touchscreen@2a {
+ reg = <0x2a>;
+ compatible = "eeti,exc80h84";
+
+ pinctrl-names = "default";
+ pinctrl-0 =<&pinctrl_q7_gpio0>;
+ interrupt-parent = <&gpio5>;
+ interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+
+ touchscreen-inverted-x;
+ touchscreen-inverted-y;
+
+ reset-gpios = <&tca6424a 21 GPIO_ACTIVE_HIGH>;
+ };
+};
diff --git a/arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi b/arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi
new file mode 100644
index 000000000000..1f9e66e1afc0
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b1x5pv2.dtsi
@@ -0,0 +1,434 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B1x5v2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+#include <dt-bindings/input/input.h>
+#include "imx6dl-qmx6.dtsi"
+
+/ {
+ chosen {
+ stdout-path = &uart3;
+ };
+
+ /* Do not allow frequencies above 800MHz */
+ cpus {
+ cpu@0 {
+ operating-points = <
+ /* kHz uV */
+ 792000 1175000
+ 396000 1150000
+ >;
+ fsl,soc-operating-points = <
+ /* ARM kHz SOC-PU uV */
+ 792000 1175000
+ 396000 1175000
+ >;
+ };
+
+ cpu@1 {
+ operating-points = <
+ /* kHz uV */
+ 792000 1175000
+ 396000 1150000
+ >;
+ fsl,soc-operating-points = <
+ /* ARM kHz SOC-PU uV */
+ 792000 1175000
+ 396000 1175000
+ >;
+ };
+ };
+
+ reg_syspwr: regulator-12v {
+ compatible = "regulator-fixed";
+ regulator-name = "SYS_PWR";
+ regulator-min-microvolt = <12000000>;
+ regulator-max-microvolt = <12000000>;
+ regulator-always-on;
+ };
+
+ reg_5v_pmc: regulator-5v-pmc {
+ compatible = "regulator-fixed";
+ regulator-name = "5V PMC";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ regulator-always-on;
+ vin-supply = <®_syspwr>;
+ };
+
+ reg_5v: regulator-5v {
+ compatible = "regulator-fixed";
+ regulator-name = "5V";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ regulator-always-on;
+ vin-supply = <®_syspwr>;
+ };
+
+ reg_3v3: regulator-3v3 {
+ compatible = "regulator-fixed";
+ regulator-name = "3V3";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ vin-supply = <®_syspwr>;
+ };
+
+ reg_5v0_audio: regulator-5v0-audio {
+ compatible = "regulator-fixed";
+ regulator-name = "5V0_AUDIO";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ vin-supply = <®_5v>;
+
+ gpio = <&tca6424a 16 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+
+ /*
+ * This must be always-on for da7212, which has some not
+ * properly documented dependencies for it's speaker supply
+ * pin. The issue manifests as speaker volume being very low.
+ */
+ regulator-always-on;
+ };
+
+
+ reg_3v3_audio: regulator-3v3-audio {
+ compatible = "regulator-fixed";
+ regulator-name = "3V3_AUDIO";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <®_3v3>;
+
+ pinctrl-0 = <&pinctrl_q7_hda_rst>;
+ pinctrl-names = "default";
+ gpio = <&gpio6 8 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
+ reg_2v5_audio: regulator-2v5-audio {
+ compatible = "regulator-fixed";
+ regulator-name = "2V5_AUDIO";
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <2500000>;
+ regulator-always-on;
+ vin-supply = <®_3v3_audio>;
+
+ };
+
+ reg_wlan: regulator-wlan {
+ compatible = "regulator-fixed";
+ regulator-name = "WLAN";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <®_3v3>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_sdio_pwr>;
+ gpio = <&gpio4 30 GPIO_ACTIVE_HIGH>;
+
+ startup-delay-us = <70000>;
+ enable-active-high;
+ };
+
+ reg_bl: regulator-backlight {
+ compatible = "regulator-fixed";
+ regulator-name = "LED_VCC";
+ regulator-min-microvolt = <12000000>;
+ regulator-max-microvolt = <12000000>;
+ vin-supply = <®_syspwr>;
+
+ pinctrl-0 = <&pinctrl_q7_lcd_power>;
+ pinctrl-names = "default";
+ gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
+ reg_lcd: regulator-lcd {
+ compatible = "regulator-fixed";
+ regulator-name = "LCD_5V";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ vin-supply = <®_5v>;
+ };
+
+ usb_power: regulator-usb-power {
+ compatible = "regulator-fixed";
+ regulator-name = "USB POWER";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ vin-supply = <®_5v>;
+ };
+
+ charger: battery-charger {
+ compatible = "gpio-charger"; /* ti,bq24172 */
+
+ charger-type = "mains";
+ gpios = <&tca6424a 3 GPIO_ACTIVE_LOW>;
+ charge-current-limit-gpios = <&tca6424a 11 GPIO_ACTIVE_HIGH>,
+ <&tca6424a 12 GPIO_ACTIVE_HIGH>;
+ charge-current-limit-mapping = <1300000 0x0>,
+ <700000 0x1>,
+ <0 0x2>;
+
+ charge-status-gpios = <&tca6424a 6 GPIO_ACTIVE_HIGH>;
+ };
+
+ poweroff {
+ compatible = "gpio-poweroff";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_spi_cs1>;
+ gpios = <&gpio4 25 GPIO_ACTIVE_LOW>;
+ };
+
+ power-button-key {
+ compatible = "gpio-keys";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_slp_btn>;
+
+ power-button {
+ label = "power button";
+ gpios = <&gpio4 7 GPIO_ACTIVE_LOW>;
+ linux,code = <KEY_POWER>;
+ };
+ };
+
+ rotary-encoder-key {
+ compatible = "gpio-keys";
+
+ rotary-encoder-press {
+ label = "rotary-encoder press";
+ gpios = <&tca6424a 0 GPIO_ACTIVE_HIGH>;
+ linux,code = <KEY_ENTER>;
+ linux,can-disable;
+ };
+ };
+
+ rotary-encoder {
+ compatible = "rotary-encoder";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_gpio2 &pinctrl_q7_gpio4>;
+ gpios = <&gpio4 26 GPIO_ACTIVE_LOW>, <&gpio1 0 GPIO_ACTIVE_LOW>;
+ rotary-encoder,relative-axis;
+ rotary-encoder,steps-per-period = <2>;
+ wakeup-source;
+ };
+
+ leds {
+ compatible = "gpio-leds";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_gpio1 &pinctrl_q7_gpio3 &pinctrl_q7_gpio5>;
+
+ alarm1 {
+ label = "alarm:red";
+ gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>;
+ };
+
+ alarm2 {
+ label = "alarm:yellow";
+ gpios = <&gpio4 27 GPIO_ACTIVE_HIGH>;
+ };
+
+ alarm3 {
+ label = "alarm:blue";
+ gpios = <&gpio4 15 GPIO_ACTIVE_HIGH>;
+ };
+ };
+
+ backlight: backlight {
+ compatible = "pwm-backlight";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_backlight_enable>;
+ power-supply = <®_bl>;
+ pwms = <&pwm4 0 5000000 0>;
+ brightness-levels = <0 255>;
+ num-interpolated-steps = <255>;
+ default-brightness-level = <179>;
+ enable-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>;
+ status = "okay";
+ };
+
+ panel {
+ backlight = <&backlight>;
+ power-supply = <®_lcd>;
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&lvds0_out>;
+ };
+ };
+ };
+
+ sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,name = "audio-card";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,bitclock-master = <&dailink_master>;
+ simple-audio-card,frame-master = <&dailink_master>;
+
+ simple-audio-card,widgets = "Speaker", "Ext Spk";
+ simple-audio-card,audio-routing = "Ext Spk", "LINE";
+
+ simple-audio-card,cpu {
+ sound-dai = <&ssi1>;
+ };
+
+ dailink_master: simple-audio-card,codec {
+ sound-dai = <&codec>;
+ };
+ };
+
+ clk_ext_audio_codec: clock-codec {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <12288000>;
+ };
+};
+
+&i2c1 {
+ battery: battery@b {
+ compatible = "ti,bq20z65", "sbs,sbs-battery";
+ reg = <0x0b>;
+
+ sbs,battery-detect-gpios = <&tca6424a 5 GPIO_ACTIVE_LOW>;
+ sbs,i2c-retry-count = <5>;
+
+ power-supplies = <&charger>;
+ };
+
+ codec: audio-codec@1a {
+ compatible = "dlg,da7212";
+ reg = <0x1a>;
+ #sound-dai-cells = <0>;
+ VDDA-supply = <®_2v5_audio>;
+ VDDSP-supply = <®_5v0_audio>;
+ VDDMIC-supply = <®_3v3_audio>;
+ VDDIO-supply = <®_3v3_audio>;
+ clocks = <&clk_ext_audio_codec>;
+ clock-names = "mclk";
+ };
+};
+
+&i2c5 {
+ tmp75: temperature-sensor@48 {
+ compatible = "ti,tmp75";
+ reg = <0x48>;
+ vs-supply = <®_3v3>;
+ interrupt-parent = <&tca6424a>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ };
+
+ tca6424a: gpio-controller@22 {
+ compatible = "ti,tca6424";
+ reg = <0x22>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ vcc-supply = <®_3v3>;
+
+ interrupt-parent = <&gpio7>;
+ interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_gpio6>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ gpio-line-names = "GPIO_ROTOR#", "ACM_IO_INT", "TMP_SENSOR_IRQ", "AC_IN",
+ "TF_S", "BATT_T", "LED_INC_CHAR", "ACM1_OCF",
+ "ACM2_OCF", "ACM_IO_RST", "USB1_POWER_EN", "EGPIO_CC_CTL0",
+ "EGPIO_CC_CTL1", "12V_OEMNBP_EN", "CP2105_RST", "",
+ "SPEAKER_PA_EN", "ARM7_UPI_RESET", "ARM7_PWR_RST", "NURSE_CALL",
+ "MARKER_EN", "EGPIO_TOUCH_RST", "PRESSURE_INT1", "PRESSURE_INT2";
+
+ };
+};
+
+&fec {
+ status = "okay";
+};
+
+&hdmi {
+ status = "okay";
+};
+
+&audmux {
+ status = "okay";
+};
+
+&ssi1 {
+ fsl,mode = "i2s-slave";
+ status = "okay";
+};
+
+&usbotg {
+ vbus-supply = <&usb_power>;
+ disable-over-current;
+ dr_mode = "host";
+ status = "okay";
+
+ /*
+ * TPS2051BDGN fault-gpio is connected to Q7[86] USB_0_1_OC_N.
+ * On QMX6 this is not connceted to the i.MX6, but to the USB Hub
+ * from &usbh1. This means, that we cannot easily detect and handle
+ * over-current events. Fortunately the regulator limits the current
+ * automatically, so the hardware is still protected.
+ */
+};
+
+&pwm4 {
+ status = "okay";
+};
+
+&ldb {
+ status = "okay";
+
+ lvds0: lvds-channel@0 {
+ status = "okay";
+ fsl,data-mapping = "spwg";
+ fsl,data-width = <24>;
+
+ port@4 {
+ reg = <4>;
+ lvds0_out: endpoint {
+ remote-endpoint = <&panel_in>;
+ };
+ };
+ };
+};
+
+&usdhc4 {
+ /* WiFi module */
+ status = "okay";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usdhc4>;
+
+ bus-width = <4>;
+ no-1-8-v;
+ non-removable;
+ wakeup-source;
+ keep-power-in-suspend;
+ cap-power-off-card;
+ max-frequency = <25000000>;
+ vmmc-supply = <®_wlan>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ wlcore: wlcore@2 {
+ compatible = "ti,wl1837";
+ reg = <2>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_q7_gpio7>;
+
+ interrupt-parent = <&gpio4>;
+ interrupts = <14 IRQ_TYPE_LEVEL_HIGH>;
+
+ tcxo-clock-frequency = <26000000>;
+ };
+
+};
diff --git a/arch/arm/boot/dts/imx6dl-b1x5v2.dtsi b/arch/arm/boot/dts/imx6dl-b1x5v2.dtsi
new file mode 100644
index 000000000000..b4c6fbd802fb
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-b1x5v2.dtsi
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for General Electric B1x5v2
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+
+#include <dt-bindings/input/input.h>
+#include "imx6dl-b1x5pv2.dtsi"
+
+/ {
+ reg_3v3_acm: regulator-3v3-acm {
+ compatible = "regulator-fixed";
+ regulator-name = "3V3 ACM";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ vin-supply = <®_3v3>;
+ };
+};
+
+&i2c1 {
+ tca6416: gpio-controller@21 {
+ compatible = "ti,tca6416";
+ reg = <0x21>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ vcc-supply = <®_3v3_acm>;
+
+ /*
+ * TCA6424 cannot handle low type interrupts at the moment and
+ * it cannot be added without quite a few hacks. Since this
+ * controller does not have any input type GPIOs, pretend that
+ * the interrupt pin is unconnected.
+ */
+ //interrupt-parent = <&tca6424a>;
+ //interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
+ //interrupt-controller;
+ //#interrupt-cells = <2>;
+
+ reset-gpios = <&tca6424a 9 GPIO_ACTIVE_LOW>;
+
+ gpio-line-names = "ACM1_EN", "ACM1_CL0", "ACM1_CL1", "ACM1_CL2",
+ "", "ACM2_EN", "ACM2_CL0", "ACM2_CL1",
+ "ACM2_CL2", "", "", "",
+ "", "", "", "";
+ };
+};
+
+&i2c5 {
+ mpl3115a2: pressure-sensor@60 {
+ compatible = "fsl,mpl3115";
+ reg = <0x60>;
+
+ vcc-supply = <®_3v3_acm>;
+
+ /* The MPL3115 binding does not yet support interrupts */
+ //interrupt-parent = <&tca6424a>;
+ //interrupts = <22 IRQ_TYPE_EDGE_RISING 23 IRQ_TYPE_EDGE_RISING>;
+ };
+};
diff --git a/arch/arm/boot/dts/imx6dl-qmx6.dtsi b/arch/arm/boot/dts/imx6dl-qmx6.dtsi
new file mode 100644
index 000000000000..dbcca0b02792
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-qmx6.dtsi
@@ -0,0 +1,623 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+//
+// Device Tree Source for i.MX6DL based congatec QMX6
+//
+// Copyright 2018-2021 General Electric Company
+// Copyright 2018-2021 Collabora
+// Copyright 2016 congatec AG
+
+#include "imx6dl.dtsi"
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/sound/fsl-imx-audmux.h>
+
+/ {
+ memory@10000000 {
+ reg = <0x10000000 0x40000000>;
+ };
+
+ reg_3p3v: 3p3v {
+ compatible = "regulator-fixed";
+ regulator-name = "3P3V";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+
+ i2cmux {
+ compatible = "i2c-mux-gpio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ mux-gpios = <&gpio6 9 GPIO_ACTIVE_HIGH>;
+ i2c-parent = <&i2c2>;
+
+ i2c5: i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
+ i2c6: i2c@1 {
+ reg = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
+};
+
+&ecspi1 {
+ fsl,spi-num-chipselects = <1>;
+ cs-gpios = <&gpio3 19 0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_ecspi1>;
+ status = "okay";
+
+ flash@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "sst,sst25vf032b", "jedec,spi-nor";
+ spi-max-frequency = <20000000>;
+ reg = <0>;
+
+ partition@0 {
+ label = "bootloader";
+ reg = <0x0000000 0x100000>;
+ };
+
+ partition@100000 {
+ label = "user";
+ reg = <0x0100000 0x2fc000>;
+ };
+
+ partition@3fc000 {
+ label = "reserved";
+ reg = <0x03fc000 0x4000>;
+ read-only;
+ };
+ };
+};
+
+&fec {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_enet &pinctrl_phy_reset>;
+ phy-mode = "rgmii-id";
+ phy-reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
+ fsl,magic-packet;
+ phy-handle = <&phy0>;
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy0: ethernet-phy@6 {
+ reg = <6>;
+ qca,clk-out-frequency = <125000000>;
+ };
+ };
+};
+
+&i2c6 {
+ pmic@8 {
+ compatible = "fsl,pfuze100";
+ reg = <0x08>;
+
+ regulators {
+ sw1a_reg: sw1ab {
+ regulator-min-microvolt = <300000>;
+ regulator-max-microvolt = <1875000>;
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-ramp-delay = <6250>;
+ };
+
+ sw1c_reg: sw1c {
+ regulator-min-microvolt = <300000>;
+ regulator-max-microvolt = <1875000>;
+ regulator-boot-on;
+ regulator-always-on;
+ regulator-ramp-delay = <6250>;
+ };
+
+ sw2_reg: sw2 {
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ sw3a_reg: sw3a {
+ regulator-min-microvolt = <400000>;
+ regulator-max-microvolt = <1975000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ sw3b_reg: sw3b {
+ regulator-min-microvolt = <400000>;
+ regulator-max-microvolt = <1975000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ sw4_reg: sw4 {
+ regulator-min-microvolt = <675000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ swbst_reg: swbst {
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5150000>;
+ };
+
+ snvs_reg: vsnvs {
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ vref_reg: vrefddr {
+ regulator-boot-on;
+ regulator-always-on;
+ };
+
+ /*
+ * keep VGEN3, VGEN4 and VGEN5 enabled in order to
+ * maintain backward compatibility with hw-rev. A.0
+ */
+ vgen3_reg: vgen3 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+
+ vgen4_reg: vgen4 {
+ regulator-min-microvolt = <2500000>;
+ regulator-max-microvolt = <2500000>;
+ regulator-always-on;
+ };
+
+ vgen5_reg: vgen5 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+
+ /* supply voltage for eMMC */
+ vgen6_reg: vgen6 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ };
+ };
+};
+
+&pcie {
+ reset-gpio = <&gpio1 20 0>;
+};
+
+&audmux {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_audmux>;
+
+ audmux_ssi1 {
+ fsl,audmux-port = <MX51_AUDMUX_PORT1_SSI0>;
+ fsl,port-config = <
+ (IMX_AUDMUX_V2_PTCR_TFSDIR |
+ IMX_AUDMUX_V2_PTCR_TFSEL(MX51_AUDMUX_PORT6) |
+ IMX_AUDMUX_V2_PTCR_TCLKDIR |
+ IMX_AUDMUX_V2_PTCR_TCSEL(MX51_AUDMUX_PORT6) |
+ IMX_AUDMUX_V2_PTCR_SYN)
+ IMX_AUDMUX_V2_PDCR_RXDSEL(MX51_AUDMUX_PORT6)
+ >;
+ };
+
+ audmux_aud6 {
+ fsl,audmux-port = <MX51_AUDMUX_PORT6>;
+ fsl,port-config = <
+ IMX_AUDMUX_V2_PTCR_SYN
+ IMX_AUDMUX_V2_PDCR_RXDSEL(MX51_AUDMUX_PORT1_SSI0)
+ >;
+ };
+};
+
+&clks {
+ assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
+ <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
+ assigned-clock-parents = <&clks IMX6QDL_CLK_PLL2_PFD0_352M>,
+ <&clks IMX6QDL_CLK_PLL2_PFD0_352M>;
+};
+
+&i2c1 {
+ clock-frequency = <100000>;
+ pinctrl-names = "default", "gpio";
+ pinctrl-0 = <&pinctrl_i2c1>;
+ pinctrl-1 = <&pinctrl_i2c1_gpio>;
+ scl-gpios = <&gpio3 21 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio3 28 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ status = "okay";
+};
+
+&i2c2 {
+ clock-frequency = <100000>;
+ pinctrl-names = "default", "gpio";
+ pinctrl-0 = <&pinctrl_i2c2>;
+ pinctrl-1 = <&pinctrl_i2c2_gpio>;
+ scl-gpios = <&gpio4 12 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio4 13 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ status = "okay";
+};
+
+&i2c3 {
+ clock-frequency = <100000>;
+ pinctrl-names = "default", "gpio";
+ pinctrl-0 = <&pinctrl_i2c3>;
+ pinctrl-1 = <&pinctrl_i2c3_gpio>;
+ scl-gpios = <&gpio1 3 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio1 6 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+ status = "okay";
+
+ rtc: m41t62@68 {
+ compatible = "st,m41t62";
+ reg = <0x68>;
+ #clocks-cells = <0>;
+ /*
+ * RTC clock provides i.MX6 CKIL and must not be disabled.
+ * Properly describing this dependency in device-tree will
+ * result in a chicken-and-egg situation, since operating
+ * system cannot control the I2C RTC without CKIL. This
+ * hardware setup only works because the RTC enables the
+ * SQW with correct 32.768 kHz by default.
+ */
+ protected-clocks;
+ };
+};
+
+&pwm4 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm4>;
+};
+
+®_arm {
+ vin-supply = <&sw1a_reg>;
+};
+
+®_pu {
+ vin-supply = <&sw1c_reg>;
+};
+
+®_soc {
+ vin-supply = <&sw1c_reg>;
+};
+
+&snvs_poweroff {
+ status = "okay";
+};
+
+&uart2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart2>;
+ status = "okay";
+};
+
+&uart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_uart3>;
+ status = "okay";
+};
+
+&usdhc2 {
+ /* MicroSD card slot */
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usdhc2>;
+ cd-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+ no-1-8-v;
+ keep-power-in-suspend;
+ wakeup-source;
+ vmmc-supply = <®_3p3v>;
+ status = "okay";
+};
+
+&usdhc3 {
+ /* eMMC module */
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usdhc3>;
+ non-removable;
+ bus-width = <8>;
+ no-1-8-v;
+ keep-power-in-suspend;
+ wakeup-source;
+ vmmc-supply = <®_3p3v>;
+ status = "okay";
+};
+
+&usbh1 {
+ /* Connected to USB-Hub SMSC USB2514, provides P0, P2, P3, P4 on Qseven connector */
+ vbus-supply = <®_5v>;
+ status = "okay";
+};
+
+&usbotg {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usbotg>;
+};
+
+&wdog1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_wdog>;
+ fsl,ext-reset-output;
+};
+
+&iomuxc {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_hog>;
+
+ qmx6mux: imx6qdl-qmx6 {
+ };
+};
+
+&qmx6mux {
+ pinctrl_hog: hoggrp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_2__GPIO1_IO02 0x80000000 /* PCIE_WAKE_B */
+ MX6QDL_PAD_NANDF_WP_B__GPIO6_IO09 0x80000000 /* I2C multiplexer */
+ MX6QDL_PAD_NANDF_D6__GPIO2_IO06 0x80000000 /* SD4_CD# */
+ MX6QDL_PAD_NANDF_D7__GPIO2_IO07 0x80000000 /* SD4_WP */
+ MX6QDL_PAD_CSI0_MCLK__CCM_CLKO1 0x80000000 /* Camera MCLK */
+ >;
+ };
+
+ /* Watchdog output signal */
+ pinctrl_wdog: wdoggrp {
+ fsl,pins = <
+ MX6QDL_PAD_DISP0_DAT8__WDOG1_B 0x1b0b0
+ >;
+ };
+
+ pinctrl_q7_hda_rst: hdarstgrp {
+ fsl,pins = <
+ MX6QDL_PAD_NANDF_ALE__GPIO6_IO08 0x1b0b0 /* Q7[61] HDA_RST_N */
+ >;
+ };
+
+ pinctrl_q7_backlight_enable: blengrp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_9__GPIO1_IO09 0x1b0b0 /* Q7[112] LVDS_BLEN */
+ >;
+ };
+
+ pinctrl_audmux: audmuxgrp {
+ fsl,pins = <
+ MX6QDL_PAD_DI0_PIN2__AUD6_TXD 0x110b0 /* Q7[67] HDA_SDO */
+ MX6QDL_PAD_DI0_PIN3__AUD6_TXFS 0x130b0 /* Q7[59] HDA_SYNC */
+ MX6QDL_PAD_DI0_PIN4__AUD6_RXD 0x130b0 /* Q7[65] HDA_SDI */
+ MX6QDL_PAD_DI0_PIN15__AUD6_TXC 0x130b0 /* Q7[63] HDA_BITCLK */
+ >;
+ };
+
+ /* SPI bus does not leave System on Module */
+ pinctrl_ecspi1: ecspi1grp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D16__ECSPI1_SCLK 0x100b1
+ MX6QDL_PAD_EIM_D17__ECSPI1_MISO 0x100b1
+ MX6QDL_PAD_EIM_D18__ECSPI1_MOSI 0x100b1
+ MX6QDL_PAD_EIM_D19__GPIO3_IO19 0x1b0b0
+ >;
+ };
+
+ /* PHY is on System on Module, Q7[3-15] have Ethernet lines */
+ pinctrl_enet: enetgrp {
+ fsl,pins = <
+ MX6QDL_PAD_ENET_MDIO__ENET_MDIO 0x1b0b0
+ MX6QDL_PAD_ENET_MDC__ENET_MDC 0x1b0b0
+ MX6QDL_PAD_RGMII_TXC__RGMII_TXC 0x1b030
+ MX6QDL_PAD_RGMII_TD0__RGMII_TD0 0x1b030
+ MX6QDL_PAD_RGMII_TD1__RGMII_TD1 0x1b030
+ MX6QDL_PAD_RGMII_TD2__RGMII_TD2 0x1b030
+ MX6QDL_PAD_RGMII_TD3__RGMII_TD3 0x1b030
+ MX6QDL_PAD_RGMII_TX_CTL__RGMII_TX_CTL 0x1b030
+ MX6QDL_PAD_ENET_REF_CLK__ENET_TX_CLK 0x1b0b0
+ MX6QDL_PAD_RGMII_RXC__RGMII_RXC 0x1b030
+ MX6QDL_PAD_RGMII_RD0__RGMII_RD0 0x1b030
+ MX6QDL_PAD_RGMII_RD1__RGMII_RD1 0x1b030
+ MX6QDL_PAD_RGMII_RD2__RGMII_RD2 0x1b030
+ MX6QDL_PAD_RGMII_RD3__RGMII_RD3 0x1b030
+ MX6QDL_PAD_RGMII_RX_CTL__RGMII_RX_CTL 0x1b030
+ MX6QDL_PAD_ENET_TX_EN__ENET_TX_EN 0x1b0b0
+ >;
+ };
+
+ /* RGMII Phy Reset */
+ pinctrl_phy_reset: phyrstgrp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D23__GPIO3_IO23 0x1b0b0
+ >;
+ };
+
+ pinctrl_i2c1: i2c1grp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D21__I2C1_SCL 0x4001b8b1 /* Q7[66] I2C_CLK */
+ MX6QDL_PAD_EIM_D28__I2C1_SDA 0x4001b8b1 /* Q7[68] I2C_DAT */
+ >;
+ };
+
+ pinctrl_i2c1_gpio: i2c1gpiogrp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D21__GPIO3_IO21 0x1b0b0 /* Q7[66] I2C_CLK */
+ MX6QDL_PAD_EIM_D28__GPIO3_IO28 0x1b0b0 /* Q7[68] I2C_DAT */
+ >;
+ };
+
+
+ pinctrl_i2c2: i2c2grp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_COL3__I2C2_SCL 0x4001b8b1 /* Q7[152] SDVO_CTRL_CLK */
+ MX6QDL_PAD_KEY_ROW3__I2C2_SDA 0x4001b8b1 /* Q7[150] SDVO_CTRL_DAT */
+ >;
+ };
+
+ pinctrl_i2c2_gpio: i2c2gpiogrp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_COL3__GPIO4_IO12 0x1b0b0 /* Q7[152] SDVO_CTRL_CLK */
+ MX6QDL_PAD_KEY_ROW3__GPIO4_IO13 0x1b0b0 /* Q7[150] SDVO_CTRL_DAT */
+ >;
+ };
+
+ pinctrl_i2c3: i2c3grp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_3__I2C3_SCL 0x4001b8b1 /* Q7[60] SMB_CLK */
+ MX6QDL_PAD_GPIO_6__I2C3_SDA 0x4001b8b1 /* Q7[62] SMB_DAT */
+ >;
+ };
+
+ pinctrl_i2c3_gpio: i2c3gpiogrp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_3__GPIO1_IO03 0x1b0b0 /* Q7[60] SMB_CLK */
+ MX6QDL_PAD_GPIO_6__GPIO1_IO06 0x1b0b0 /* Q7[62] SMB_DAT */
+ >;
+ };
+
+ pinctrl_pwm4: pwm4grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD1_CMD__PWM4_OUT 0x1b0b1 /* Q7[123] LVDS_BLT_CTRL */
+ >;
+ };
+
+ /* Debug connector on Q7 module */
+ pinctrl_uart2: uart2grp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D26__UART2_TX_DATA 0x1b0b1
+ MX6QDL_PAD_EIM_D27__UART2_RX_DATA 0x1b0b1
+ >;
+ };
+
+ pinctrl_uart3: uart3grp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_D25__UART3_RX_DATA 0x1b0b1 /* Q7[177] UART0_RX */
+ MX6QDL_PAD_EIM_D24__UART3_TX_DATA 0x1b0b1 /* Q7[171] UART0_TX */
+ >;
+ };
+
+ /* µSD card slot on Q7 module */
+ pinctrl_usdhc2: usdhc2grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD2_CMD__SD2_CMD 0x17059
+ MX6QDL_PAD_SD2_CLK__SD2_CLK 0x10059
+ MX6QDL_PAD_SD2_DAT0__SD2_DATA0 0x17059
+ MX6QDL_PAD_SD2_DAT1__SD2_DATA1 0x17059
+ MX6QDL_PAD_SD2_DAT2__SD2_DATA2 0x17059
+ MX6QDL_PAD_SD2_DAT3__SD2_DATA3 0x17059
+ MX6QDL_PAD_GPIO_4__GPIO1_IO04 0x1b0b0 /* SD2_CD */
+ >;
+ };
+
+ /* eMMC module on Q7 module */
+ pinctrl_usdhc3: usdhc3grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD3_CMD__SD3_CMD 0x17059
+ MX6QDL_PAD_SD3_CLK__SD3_CLK 0x10059
+ MX6QDL_PAD_SD3_DAT0__SD3_DATA0 0x17059
+ MX6QDL_PAD_SD3_DAT1__SD3_DATA1 0x17059
+ MX6QDL_PAD_SD3_DAT2__SD3_DATA2 0x17059
+ MX6QDL_PAD_SD3_DAT3__SD3_DATA3 0x17059
+ MX6QDL_PAD_SD3_DAT4__SD3_DATA4 0x17059
+ MX6QDL_PAD_SD3_DAT5__SD3_DATA5 0x17059
+ MX6QDL_PAD_SD3_DAT6__SD3_DATA6 0x17059
+ MX6QDL_PAD_SD3_DAT7__SD3_DATA7 0x17059
+ >;
+ };
+
+
+ pinctrl_usdhc4: usdhc4grp {
+ fsl,pins = <
+ MX6QDL_PAD_SD4_CMD__SD4_CMD 0x17059 /* Q7[45] SDIO_CMD */
+ MX6QDL_PAD_SD4_CLK__SD4_CLK 0x17059 /* Q7[42] SDIO_CLK */
+ MX6QDL_PAD_SD4_DAT1__SD4_DATA1 0x17059 /* Q7[48] SDIO_DAT1 */
+ MX6QDL_PAD_SD4_DAT0__SD4_DATA0 0x17059 /* Q7[49] SDIO_DAT0 */
+ MX6QDL_PAD_SD4_DAT3__SD4_DATA3 0x17059 /* Q7[50] SDIO_DAT3 */
+ MX6QDL_PAD_SD4_DAT2__SD4_DATA2 0x17059 /* Q7[51] SDIO_DAT2 */
+ >;
+ };
+
+ pinctrl_q7_slp_btn: q7slpbtngrp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_ROW0__GPIO4_IO07 0x1b0b0 /* Q7[21] SLP_BTN# */
+ >;
+ };
+
+ pinctrl_q7_sdio_pwr: q7sdiopwrgrp {
+ fsl,pins = <
+ MX6QDL_PAD_DISP0_DAT9__GPIO4_IO30 0x1b0b0 /* Q7[47] SDIO_PWR# */
+ >;
+ };
+
+ pinctrl_q7_gpio0: q7gpio0grp {
+ fsl,pins = <
+ MX6QDL_PAD_EIM_A25__GPIO5_IO02 0x1b0b0 /* Q7[185] GPIO0 */
+ >;
+ };
+
+ pinctrl_q7_gpio1: q7gpio1grp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_8__GPIO1_IO08 0x1b0b0 /* Q7[186] GPIO1 */
+ >;
+ };
+
+ pinctrl_q7_gpio2: q7gpio2grp {
+ fsl,pins = <
+ MX6QDL_PAD_DISP0_DAT5__GPIO4_IO26 0x1b0b0 /* Q7[187] GPIO2 */
+ >;
+ };
+
+ pinctrl_q7_gpio3: q7gpio3grp {
+ fsl,pins = <
+ MX6QDL_PAD_DISP0_DAT6__GPIO4_IO27 0x1b0b0 /* Q7[188] GPIO3 */
+ >;
+ };
+
+ pinctrl_q7_gpio4: q7gpio4grp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_0__GPIO1_IO00 0x1b0b0 /* Q7[189] GPIO4 */
+ >;
+ };
+
+ pinctrl_q7_gpio5: q7gpio5grp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_ROW4__GPIO4_IO15 0x1b0b0 /* Q7[190] GPIO5 */
+ >;
+ };
+
+ pinctrl_q7_gpio6: q7gpio6grp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_16__GPIO7_IO11 0x1b0b0 /* Q7[191] GPIO6 */
+ >;
+ };
+
+ pinctrl_q7_gpio7: q7gpio7grp {
+ fsl,pins = <
+ MX6QDL_PAD_KEY_COL4__GPIO4_IO14 0x1b0b0 /* Q7[192] GPIO7 */
+ >;
+ };
+
+ pinctrl_usbotg: usbotggrp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_1__USB_OTG_ID 0x17059 /* Q7[92] USB_ID */
+ >;
+ };
+
+ pinctrl_q7_lcd_power: lcdpwrgrp {
+ fsl,pins = <
+ MX6QDL_PAD_GPIO_7__GPIO1_IO07 0x1b0b0 /* Q7[111] LVDS_PPEN */
+ >;
+ };
+
+ pinctrl_q7_spi_cs1: spics1grp {
+ fsl,pins = <
+ MX6QDL_PAD_DISP0_DAT4__GPIO4_IO25 0x1b0b0 /* Q7[202] SPI_CS1# */
+ >;
+ };
+};
--
2.30.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-02-22 17:12 ` [PATCHv1 1/6] rtc: m41t80: add support for protected clock Sebastian Reichel
@ 2021-02-22 21:20 ` Alexandre Belloni
2021-02-22 21:26 ` Alexandre Belloni
0 siblings, 1 reply; 28+ messages in thread
From: Alexandre Belloni @ 2021-02-22 21:20 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Philipp Zabel, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, NXP Linux Team, Rob Herring, Alessandro Zummo,
David Airlie, Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
On 22/02/2021 18:12:42+0100, Sebastian Reichel wrote:
> Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> modules SQW clock output defaults to 32768 Hz. This behaviour is
> used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> the clock is disabled and all i.MX6 functionality depending on
> the 32 KHz clock has undefined behaviour. On systems using hardware
> watchdog it seems to likely trigger a lot earlier than configured.
>
> The proper solution would be to describe this dependency in DT,
> but that will result in a deadlock. The kernel will see, that
> i.MX6 system clock needs the RTC clock and do probe deferral.
> But the i.MX6 I2C module never becomes usable without the i.MX6
> CKIL clock and thus the RTC's clock will not be probed. So from
> the kernel's perspective this is a chicken-and-egg problem.
>
Reading the previous paragraph, I was going to suggest describing the
dependency and wondering whether this would cause a circular dependency.
I guess this will keep being an issue for clocks on an I2C or SPI bus...
> Technically everything is fine by not touching anything, since
> the RTC clock correctly enables the clock on reset (i.e. on
> battery backup power loss) and also the bootloader enables it
> in case a kernel without this support has been booted.
>
> The 'protected-clocks' property is already in use for some clocks
> that may not be touched because of firmware limitations and is
> described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
> Documentation/devicetree/bindings/rtc/rtc-m41t80.txt | 1 +
> drivers/rtc/rtc-m41t80.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
> index c746cb221210..ea4bbf5c4282 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
> +++ b/Documentation/devicetree/bindings/rtc/rtc-m41t80.txt
> @@ -19,6 +19,7 @@ Optional properties:
> - interrupts: rtc alarm interrupt.
> - clock-output-names: From common clock binding to override the default output
> clock name
> +- protected-clocks: Bool, if set operating system should not handle clock.
> - wakeup-source: Enables wake up of host system on alarm
>
> Example:
> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 160dcf68e64e..3296583853a8 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -546,6 +546,9 @@ static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
> struct clk_init_data init;
> int ret;
>
> + if (of_property_read_bool(node, "protected-clocks"))
> + return 0;
> +
> /* First disable the clock */
> ret = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_MON);
> if (ret < 0)
> --
> 2.30.0
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-02-22 21:20 ` Alexandre Belloni
@ 2021-02-22 21:26 ` Alexandre Belloni
2021-02-23 1:26 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Alexandre Belloni @ 2021-02-22 21:26 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Philipp Zabel, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, NXP Linux Team, Rob Herring, Alessandro Zummo,
David Airlie, Daniel Vetter, Miquel Raynal, devicetree, dri-devel,
linux-arm-kernel, linux-kernel, linux-rtc, linux-mtd, kernel
On 22/02/2021 22:20:47+0100, Alexandre Belloni wrote:
> On 22/02/2021 18:12:42+0100, Sebastian Reichel wrote:
> > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > the clock is disabled and all i.MX6 functionality depending on
> > the 32 KHz clock has undefined behaviour. On systems using hardware
> > watchdog it seems to likely trigger a lot earlier than configured.
> >
> > The proper solution would be to describe this dependency in DT,
> > but that will result in a deadlock. The kernel will see, that
> > i.MX6 system clock needs the RTC clock and do probe deferral.
> > But the i.MX6 I2C module never becomes usable without the i.MX6
> > CKIL clock and thus the RTC's clock will not be probed. So from
> > the kernel's perspective this is a chicken-and-egg problem.
> >
>
> Reading the previous paragraph, I was going to suggest describing the
> dependency and wondering whether this would cause a circular dependency.
> I guess this will keep being an issue for clocks on an I2C or SPI bus...
>
> > Technically everything is fine by not touching anything, since
> > the RTC clock correctly enables the clock on reset (i.e. on
> > battery backup power loss) and also the bootloader enables it
> > in case a kernel without this support has been booted.
> >
> > The 'protected-clocks' property is already in use for some clocks
> > that may not be touched because of firmware limitations and is
> > described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Or maybe you expected me to apply the patch, how are the following
patches dependent on this one?
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b
2021-02-22 17:12 ` [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b Sebastian Reichel
@ 2021-02-23 0:15 ` Rob Herring
2021-02-23 1:33 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Rob Herring @ 2021-02-23 0:15 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Philipp Zabel, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, NXP Linux Team, Alexandre Belloni,
Alessandro Zummo, David Airlie, Daniel Vetter, Miquel Raynal,
devicetree, dri-devel, linux-arm-kernel,
linux-kernel@vger.kernel.org,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM, MTD Maling List,
Collabora Kernel ML
On Mon, Feb 22, 2021 at 11:13 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> The binding is already used by the driver. Update documentation
> accordingly.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 1 +
> 1 file changed, 1 insertion(+)
This is now DT schema format. Landed in Linus' tree today.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-02-22 21:26 ` Alexandre Belloni
@ 2021-02-23 1:26 ` Sebastian Reichel
2021-03-06 19:56 ` Rob Herring
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-23 1:26 UTC (permalink / raw)
To: Alexandre Belloni
Cc: linux-rtc, Alessandro Zummo, Philipp Zabel, devicetree,
David Airlie, Shawn Guo, Sascha Hauer, linux-kernel, dri-devel,
Rob Herring, linux-mtd, NXP Linux Team, Pengutronix Kernel Team,
Miquel Raynal, Daniel Vetter, kernel, Fabio Estevam,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]
Hi,
On Mon, Feb 22, 2021 at 10:26:26PM +0100, Alexandre Belloni wrote:
> On 22/02/2021 22:20:47+0100, Alexandre Belloni wrote:
> > On 22/02/2021 18:12:42+0100, Sebastian Reichel wrote:
> > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > the clock is disabled and all i.MX6 functionality depending on
> > > the 32 KHz clock has undefined behaviour. On systems using hardware
> > > watchdog it seems to likely trigger a lot earlier than configured.
> > >
> > > The proper solution would be to describe this dependency in DT,
> > > but that will result in a deadlock. The kernel will see, that
> > > i.MX6 system clock needs the RTC clock and do probe deferral.
> > > But the i.MX6 I2C module never becomes usable without the i.MX6
> > > CKIL clock and thus the RTC's clock will not be probed. So from
> > > the kernel's perspective this is a chicken-and-egg problem.
> > >
> >
> > Reading the previous paragraph, I was going to suggest describing the
> > dependency and wondering whether this would cause a circular dependency.
> > I guess this will keep being an issue for clocks on an I2C or SPI bus...
Yes, it is a circular dependency on this particular system on
module. It only works because the RTC enables the clock by
default. The i.MX6 CKIL is expected to be always enabled.
> > > Technically everything is fine by not touching anything, since
> > > the RTC clock correctly enables the clock on reset (i.e. on
> > > battery backup power loss) and also the bootloader enables it
> > > in case a kernel without this support has been booted.
> > >
> > > The 'protected-clocks' property is already in use for some clocks
> > > that may not be touched because of firmware limitations and is
> > > described in Documentation/devicetree/bindings/clock/clock-bindings.txt.
> > >
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> Or maybe you expected me to apply the patch, how are the following
> patches dependent on this one?
The last patch, which introduces a new board has a runtime
dependency on this patch. Without this feature the board
goes into a reboot loop because its bootloader enables the
i.MX6 watchdog and without the CKIL its timing is messed up.
But it's a pure runtime dependency for a new board, so it should
be fine to merge this via your tree. It basically means the board
is only working once your tree and arm tree have been merged,
which seems ok from my POV.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b
2021-02-23 0:15 ` Rob Herring
@ 2021-02-23 1:33 ` Sebastian Reichel
0 siblings, 0 replies; 28+ messages in thread
From: Sebastian Reichel @ 2021-02-23 1:33 UTC (permalink / raw)
To: Rob Herring
Cc: open list:REAL TIME CLOCK (RTC) SUBSYSTEM, Alessandro Zummo,
Alexandre Belloni, devicetree, David Airlie, Shawn Guo,
Sascha Hauer, linux-kernel@vger.kernel.org, dri-devel,
MTD Maling List, NXP Linux Team, Pengutronix Kernel Team,
Miquel Raynal, Collabora Kernel ML, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 622 bytes --]
Hi,
On Mon, Feb 22, 2021 at 06:15:11PM -0600, Rob Herring wrote:
> On Mon, Feb 22, 2021 at 11:13 AM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > The binding is already used by the driver. Update documentation
> > accordingly.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 1 +
> > 1 file changed, 1 insertion(+)
>
> This is now DT schema format. Landed in Linus' tree today.
Indeed and it already contains sst,sst25vf032b.
This patch can be ignored.
Thanks,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-02-23 1:26 ` Sebastian Reichel
@ 2021-03-06 19:56 ` Rob Herring
2021-03-08 14:03 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Rob Herring @ 2021-03-06 19:56 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Alexandre Belloni, linux-rtc, Alessandro Zummo, Philipp Zabel,
devicetree, David Airlie, Shawn Guo, Sascha Hauer, linux-kernel,
dri-devel, linux-mtd, NXP Linux Team, Pengutronix Kernel Team,
Miquel Raynal, Daniel Vetter, kernel, Fabio Estevam,
linux-arm-kernel
On Tue, Feb 23, 2021 at 02:26:57AM +0100, Sebastian Reichel wrote:
> Hi,
>
> On Mon, Feb 22, 2021 at 10:26:26PM +0100, Alexandre Belloni wrote:
> > On 22/02/2021 22:20:47+0100, Alexandre Belloni wrote:
> > > On 22/02/2021 18:12:42+0100, Sebastian Reichel wrote:
> > > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > > the clock is disabled and all i.MX6 functionality depending on
> > > > the 32 KHz clock has undefined behaviour. On systems using hardware
> > > > watchdog it seems to likely trigger a lot earlier than configured.
> > > >
> > > > The proper solution would be to describe this dependency in DT,
> > > > but that will result in a deadlock. The kernel will see, that
> > > > i.MX6 system clock needs the RTC clock and do probe deferral.
> > > > But the i.MX6 I2C module never becomes usable without the i.MX6
> > > > CKIL clock and thus the RTC's clock will not be probed. So from
> > > > the kernel's perspective this is a chicken-and-egg problem.
> > > >
> > >
> > > Reading the previous paragraph, I was going to suggest describing the
> > > dependency and wondering whether this would cause a circular dependency.
> > > I guess this will keep being an issue for clocks on an I2C or SPI bus...
>
> Yes, it is a circular dependency on this particular system on
> module. It only works because the RTC enables the clock by
> default. The i.MX6 CKIL is expected to be always enabled.
I think you should describe the circular clocking and then provide a way
to break the dependency. It's a somewhat common issue.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 3/6] dt-bindings: vendor-prefixes: add congatec
2021-02-22 17:12 ` [PATCHv1 3/6] dt-bindings: vendor-prefixes: add congatec Sebastian Reichel
@ 2021-03-06 19:57 ` Rob Herring
0 siblings, 0 replies; 28+ messages in thread
From: Rob Herring @ 2021-03-06 19:57 UTC (permalink / raw)
To: Sebastian Reichel
Cc: linux-rtc, devicetree, linux-arm-kernel, Philipp Zabel,
Sascha Hauer, Shawn Guo, Alexandre Belloni,
Pengutronix Kernel Team, dri-devel, Fabio Estevam, NXP Linux Team,
David Airlie, Rob Herring, Miquel Raynal, linux-mtd, linux-kernel,
kernel, Alessandro Zummo
On Mon, 22 Feb 2021 18:12:44 +0100, Sebastian Reichel wrote:
> Document binding for congatec.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 4/6] dt-bindings: arm: fsl: add GE B1x5pv2 boards
2021-02-22 17:12 ` [PATCHv1 4/6] dt-bindings: arm: fsl: add GE B1x5pv2 boards Sebastian Reichel
@ 2021-03-06 19:58 ` Rob Herring
0 siblings, 0 replies; 28+ messages in thread
From: Rob Herring @ 2021-03-06 19:58 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Alessandro Zummo, linux-arm-kernel, Sascha Hauer, David Airlie,
Shawn Guo, Rob Herring, linux-kernel, devicetree, Fabio Estevam,
NXP Linux Team, linux-mtd, linux-rtc, Philipp Zabel, kernel,
Pengutronix Kernel Team, Miquel Raynal, dri-devel,
Alexandre Belloni
On Mon, 22 Feb 2021 18:12:45 +0100, Sebastian Reichel wrote:
> Document the compatible for GE B1x5pv2 boards.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> Documentation/devicetree/bindings/arm/fsl.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-03-06 19:56 ` Rob Herring
@ 2021-03-08 14:03 ` Sebastian Reichel
2021-03-16 21:51 ` Rob Herring
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-03-08 14:03 UTC (permalink / raw)
To: Rob Herring
Cc: Alexandre Belloni, linux-rtc, Alessandro Zummo, Philipp Zabel,
devicetree, David Airlie, Shawn Guo, Sascha Hauer, linux-kernel,
dri-devel, linux-mtd, NXP Linux Team, Pengutronix Kernel Team,
Miquel Raynal, Daniel Vetter, kernel, Fabio Estevam,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 4777 bytes --]
Hi,
On Sat, Mar 06, 2021 at 11:56:45AM -0800, Rob Herring wrote:
> On Tue, Feb 23, 2021 at 02:26:57AM +0100, Sebastian Reichel wrote:
> > On Mon, Feb 22, 2021 at 10:26:26PM +0100, Alexandre Belloni wrote:
> > > On 22/02/2021 22:20:47+0100, Alexandre Belloni wrote:
> > > > On 22/02/2021 18:12:42+0100, Sebastian Reichel wrote:
> > > > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > > > the clock is disabled and all i.MX6 functionality depending on
> > > > > the 32 KHz clock has undefined behaviour. On systems using hardware
> > > > > watchdog it seems to likely trigger a lot earlier than configured.
> > > > >
> > > > > The proper solution would be to describe this dependency in DT,
> > > > > but that will result in a deadlock. The kernel will see, that
> > > > > i.MX6 system clock needs the RTC clock and do probe deferral.
> > > > > But the i.MX6 I2C module never becomes usable without the i.MX6
> > > > > CKIL clock and thus the RTC's clock will not be probed. So from
> > > > > the kernel's perspective this is a chicken-and-egg problem.
> > > > >
> > > >
> > > > Reading the previous paragraph, I was going to suggest describing the
> > > > dependency and wondering whether this would cause a circular dependency.
> > > > I guess this will keep being an issue for clocks on an I2C or SPI bus...
> >
> > Yes, it is a circular dependency on this particular system on
> > module. It only works because the RTC enables the clock by
> > default. The i.MX6 CKIL is expected to be always enabled.
>
> I think you should describe the circular clocking and then provide a way
> to break the dependency.
This is very much not trivial. The clock is required during early
initialization of the i.MX. At this point we are far from probing
I2C drivers and without the I2C driver the clock is not registered.
The current i.MX code expects the system clocks to be fixed clocks,
since they must be enabled before any code is executed (incl.
bootloader) and must never be disabled. From a HW design point of
view it does not make sense to have a SW controllable clock for it,
since it just adds extra cost. I believe for QMX6 it is only SW
controllable, because that avoids the need for an extra crystal.
So how is the clock framework supposed to know, that it can ignore
the clock during registration? I see the following options:
1. My solution is the simplest one. Keep i.MX clock code the same
(it assumes a fixed-clock being used for CKIL) and avoid
registering RTC clock. This basically means the RTC is considered
to be a fixed-clock on this system, which is what the HW designers
seemed to have in mind (vendor kernel for the QMX6 is old enough
(4.9.x) to not to have CLK feature in the RTC driver. Vendor
U-Boot also does not touch the RTC. Booting mainline kernel once
bricks QMX6 boards until RTC battery is removed, so one could
actually argue addition of the CLK feature in 1373e77b4f10 (4.13)
is a regression). Currently Qualcomm device uses "protected-clocks"
for FW controlled clocks where Linux would crash the system by
trying to access them. IMHO the RTC is similar, since disabling
or modifying its frequency on QMX6 results in undefined behaviour
and possibly system crash.
2. Make i.MX clock code use the RTC as CKIL clock provider, but
ignore it somehow. I see three sub-options:
2.1. Add a property 'boot-enabled' to the RTC node, so that the
clock framework is aware of clock being enabled. This can
be used to satisfy clock dependencies somehow.
2.2. The RTC device is not probed without I2C bus, but the driver
could also register a fake clock purely based on DT
information by adding some early init hook and take over
the clock once the I2C part is being probed. I think this
is a bad idea regarding maintainability of the driver.
Also for systems not using the RTC clock, the early clock
registration is basically wrong: If the kernel disables
the RTC it will stay disabled across boots if the RTC has
a backup battery. Basically we cannot imply anything from
the RTC compatible value alone.
2.3 The i.MX core code could request CKIL with some flag, that
it's fine to have an unresolvable clock and just expect it
to be boot-enabled. The rationale would be, that CKIL must
be always-enabled.
> It's a somewhat common issue.
It is? This only works, because one can treat the RTC's clock
output like a fixed clock by not messing around with it.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCHv1 1/6] rtc: m41t80: add support for protected clock
2021-03-08 14:03 ` Sebastian Reichel
@ 2021-03-16 21:51 ` Rob Herring
2021-03-18 21:03 ` [RFC] clk: add boot clock support Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Rob Herring @ 2021-03-16 21:51 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Alexandre Belloni, linux-rtc, Alessandro Zummo, Philipp Zabel,
devicetree, David Airlie, Shawn Guo, Sascha Hauer, linux-kernel,
dri-devel, linux-mtd, NXP Linux Team, Pengutronix Kernel Team,
Miquel Raynal, Daniel Vetter, kernel, Fabio Estevam,
linux-arm-kernel
On Mon, Mar 08, 2021 at 03:03:58PM +0100, Sebastian Reichel wrote:
> Hi,
>
> On Sat, Mar 06, 2021 at 11:56:45AM -0800, Rob Herring wrote:
> > On Tue, Feb 23, 2021 at 02:26:57AM +0100, Sebastian Reichel wrote:
> > > On Mon, Feb 22, 2021 at 10:26:26PM +0100, Alexandre Belloni wrote:
> > > > On 22/02/2021 22:20:47+0100, Alexandre Belloni wrote:
> > > > > On 22/02/2021 18:12:42+0100, Sebastian Reichel wrote:
> > > > > > Congatec's QMX6 system on module (SoM) uses a m41t62 as RTC. The
> > > > > > modules SQW clock output defaults to 32768 Hz. This behaviour is
> > > > > > used to provide the i.MX6 CKIL clock. Once the RTC driver is probed,
> > > > > > the clock is disabled and all i.MX6 functionality depending on
> > > > > > the 32 KHz clock has undefined behaviour. On systems using hardware
> > > > > > watchdog it seems to likely trigger a lot earlier than configured.
> > > > > >
> > > > > > The proper solution would be to describe this dependency in DT,
> > > > > > but that will result in a deadlock. The kernel will see, that
> > > > > > i.MX6 system clock needs the RTC clock and do probe deferral.
> > > > > > But the i.MX6 I2C module never becomes usable without the i.MX6
> > > > > > CKIL clock and thus the RTC's clock will not be probed. So from
> > > > > > the kernel's perspective this is a chicken-and-egg problem.
> > > > > >
> > > > >
> > > > > Reading the previous paragraph, I was going to suggest describing the
> > > > > dependency and wondering whether this would cause a circular dependency.
> > > > > I guess this will keep being an issue for clocks on an I2C or SPI bus...
> > >
> > > Yes, it is a circular dependency on this particular system on
> > > module. It only works because the RTC enables the clock by
> > > default. The i.MX6 CKIL is expected to be always enabled.
> >
> > I think you should describe the circular clocking and then provide a way
> > to break the dependency.
>
> This is very much not trivial. The clock is required during early
> initialization of the i.MX. At this point we are far from probing
> I2C drivers and without the I2C driver the clock is not registered.
> The current i.MX code expects the system clocks to be fixed clocks,
> since they must be enabled before any code is executed (incl.
> bootloader) and must never be disabled. From a HW design point of
> view it does not make sense to have a SW controllable clock for it,
> since it just adds extra cost. I believe for QMX6 it is only SW
> controllable, because that avoids the need for an extra crystal.
>
> So how is the clock framework supposed to know, that it can ignore
> the clock during registration? I see the following options:
>
> 1. My solution is the simplest one. Keep i.MX clock code the same
> (it assumes a fixed-clock being used for CKIL) and avoid
> registering RTC clock. This basically means the RTC is considered
> to be a fixed-clock on this system, which is what the HW designers
> seemed to have in mind (vendor kernel for the QMX6 is old enough
> (4.9.x) to not to have CLK feature in the RTC driver. Vendor
> U-Boot also does not touch the RTC. Booting mainline kernel once
> bricks QMX6 boards until RTC battery is removed, so one could
> actually argue addition of the CLK feature in 1373e77b4f10 (4.13)
> is a regression). Currently Qualcomm device uses "protected-clocks"
> for FW controlled clocks where Linux would crash the system by
> trying to access them. IMHO the RTC is similar, since disabling
> or modifying its frequency on QMX6 results in undefined behaviour
> and possibly system crash.
>
> 2. Make i.MX clock code use the RTC as CKIL clock provider, but
> ignore it somehow. I see three sub-options:
>
> 2.1. Add a property 'boot-enabled' to the RTC node, so that the
> clock framework is aware of clock being enabled. This can
> be used to satisfy clock dependencies somehow.
>
> 2.2. The RTC device is not probed without I2C bus, but the driver
> could also register a fake clock purely based on DT
> information by adding some early init hook and take over
> the clock once the I2C part is being probed. I think this
> is a bad idea regarding maintainability of the driver.
> Also for systems not using the RTC clock, the early clock
> registration is basically wrong: If the kernel disables
> the RTC it will stay disabled across boots if the RTC has
> a backup battery. Basically we cannot imply anything from
> the RTC compatible value alone.
>
> 2.3 The i.MX core code could request CKIL with some flag, that
> it's fine to have an unresolvable clock and just expect it
> to be boot-enabled. The rationale would be, that CKIL must
> be always-enabled.
I think 2.1 or 2.3 is fine. It boils down to detecting a cycle and then
either you have a property or implicitly know to ignore a dependency.
> > It's a somewhat common issue.
>
> It is? This only works, because one can treat the RTC's clock
> output like a fixed clock by not messing around with it.
Well, it's not the first time I've heard of the issue. Audio clocks are
another example, but a bit different in that the clocks aren't needed
until later. It's also come up in context of fw_devlinks which I
think has some cycle breaking logic already.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* [RFC] clk: add boot clock support
2021-03-16 21:51 ` Rob Herring
@ 2021-03-18 21:03 ` Sebastian Reichel
2021-03-26 1:27 ` Rob Herring
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-03-18 21:03 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
devicetree, linux-kernel, kernel
On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
is provided by an I2C RTC. Specifying this properly results in a
circular dependency, since the I2C RTC (and thus its clock) cannot
be initialized without the i.MX6 clock controller being initialized.
With current code the following path is executed when i.MX6 clock
controller is probed (and ckil clock is specified to be the I2C RTC
via DT):
1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
2. of_clk_get_by_name(ccm_node, "ckil");
3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
4. of_clk_get_hw(ccm_node, 0, "ckil")
5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
7. error is propagated back, i.MX6q clock controller is probe deferred
8. I2C controller is never initialized without clock controller
I2C RTC is never initialized without I2C controller
CKIL clock is never initialized without I2C RTC
clock controller is never initialized without CKIL
To fix the circular dependency this registers a dummy clock when
the RTC clock is tried to be acquired. The dummy clock will later
be unregistered when the proper clock is registered for the RTC
DT node. IIUIC clk_core_reparent_orphans() will take care of
fixing up the clock tree.
NOTE: For now the patch is compile tested only. If this approach
is the correct one I will do some testing and properly submit this.
You can find all the details about the hardware in the following
patchset:
https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
.../bindings/clock/clock-bindings.txt | 7 +
drivers/clk/clk.c | 146 ++++++++++++++++++
2 files changed, 153 insertions(+)
diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index f2ea53832ac6..66d67ff4aa0f 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
Clock consumer nodes must never directly reference
the provider's clock-output-names property.
+boot-clock-frequencies: This property is used to specify that a clock is enabled
+ by default with the provided frequency at boot time. This
+ is required to break circular clock dependencies. For clock
+ providers with #clock-cells = 0 this is a single u32
+ with the frequency in Hz. Otherwise it's a list of
+ clock cell specifier + frequency in Hz.
+
For example:
oscillator {
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5052541a0986..029088ed5f1a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4499,6 +4499,146 @@ static const struct of_device_id __clk_of_table_sentinel
static LIST_HEAD(of_clk_providers);
static DEFINE_MUTEX(of_clk_mutex);
+struct of_boot_clk {
+ struct of_clk_provider cp;
+ struct clk_hw clk;
+ unsigned long rate;
+};
+
+/**
+ * of_clk_get_boot_rate() - Get DT configured boot rate for a DT clkspec
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function provides the boot clock rate configured in DT
+ * for a clkspec without requiring a device being registered in
+ * the kernel.
+ *
+ * This is required for clock setups with circular dependencies,
+ * which only work because of some clocks being enabled
+ * automatically.
+ *
+ * The return value is either the rate,
+ * -EINVAL for malformed DT,
+ * -ENODATA if no boot frequency is specified.
+ */
+static int of_clk_get_boot_rate(struct of_phandle_args *clkspec)
+{
+ const struct device_node *np;
+ u32 cells;
+ u32 val;
+
+ if (!clkspec || !clkspec->np)
+ return -EINVAL;
+ np = clkspec->np;
+
+ if (!of_property_read_u32(np, "#clock-cells", &cells))
+ return -EINVAL;
+
+ /* complex clock providers are currently not supported */
+ if (cells > 0)
+ return -EINVAL;
+
+ if (!of_property_read_u32(np, "boot-clock-frequencies", &val))
+ return -ENODATA;
+
+ return val;
+}
+
+static struct clk_hw *of_boot_clk_get(struct of_phandle_args *clkspec, void *data)
+{
+ return data;
+}
+
+static unsigned long
+of_boot_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+ struct of_boot_clk *bootclk = container_of(hw, struct of_boot_clk, clk);
+
+ return bootclk->rate;
+}
+
+static const struct clk_ops of_boot_clk_ops = {
+ .recalc_rate = of_boot_clk_recalc_rate,
+};
+
+/**
+ * of_clk_register_boot_clk() - Register a boot clock provider for a node
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * Register a fixed rate dummy clock for solving circular dependencies
+ * during boot. This is expected to be replaced by a real clock device
+ * once the correct driver is probed.
+ *
+ * The function expects of_clk_mutex to be locked.
+ *
+ * Returns:
+ * -EPROBE_DEFER, if DT does not specify a boot clock
+ * -ENOMEM, if there is not sufficient memory available
+ * -EINVAL, if DT contains invalid data
+ * dummy clk_hw device on success
+ */
+static struct clk_hw *
+of_clk_register_boot_clk(struct of_phandle_args *clkspec)
+{
+ struct of_boot_clk *bootclk;
+ struct clk_init_data init;
+ int rate = of_clk_get_boot_rate(clkspec);
+
+ WARN_ON(!mutex_is_locked(&of_clk_mutex));
+
+ if (rate < 0) {
+ if (rate == -ENODATA)
+ return ERR_PTR(-EPROBE_DEFER);
+ return ERR_PTR(rate);
+ }
+
+ bootclk = kzalloc(sizeof(*bootclk), GFP_KERNEL);
+ if (!bootclk)
+ return ERR_PTR(-ENOMEM);
+
+ bootclk->rate = rate;
+
+ /* TODO: name should be unique, use idr_alloc */
+ init.name = "dummy-boot-clk";
+ init.ops = &of_boot_clk_ops;
+ init.flags = 0;
+ init.parent_names = NULL;
+ init.num_parents = 0;
+ bootclk->clk.init = &init;
+
+ bootclk->cp.node = of_node_get(clkspec->np);
+ bootclk->cp.data = bootclk;
+ bootclk->cp.get_hw = of_boot_clk_get;
+
+ /* TODO: use same name as in clk.init.name */
+ clk_hw_register_clkdev(&bootclk->clk, NULL, "dummy-boot-clk");
+
+ list_add(&bootclk->cp.link, &of_clk_providers);
+
+ pr_debug("Added clk_hw boot provider from %pOF\n", clkspec->np);
+
+ return &bootclk->clk;
+}
+
+static void of_clk_unregister_boot_clk(struct device_node *np)
+{
+ struct of_boot_clk *bootclk;
+ struct of_clk_provider *cp;
+
+ WARN_ON(!mutex_is_locked(&of_clk_mutex));
+
+ list_for_each_entry(cp, &of_clk_providers, link) {
+ if (cp->node == np && cp->get_hw == of_boot_clk_get) {
+ bootclk = container_of(cp, struct of_boot_clk, cp);
+ list_del(&cp->link);
+ // TODO: undo clk_hw_register_clkdev
+ of_node_put(cp->node);
+ kfree(bootclk);
+ break;
+ }
+ }
+}
+
struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
void *data)
{
@@ -4566,6 +4706,7 @@ int of_clk_add_provider(struct device_node *np,
cp->get = clk_src_get;
mutex_lock(&of_clk_mutex);
+ of_clk_unregister_boot_clk(np);
list_add(&cp->link, &of_clk_providers);
mutex_unlock(&of_clk_mutex);
pr_debug("Added clock from %pOF\n", np);
@@ -4605,6 +4746,7 @@ int of_clk_add_hw_provider(struct device_node *np,
cp->get_hw = get;
mutex_lock(&of_clk_mutex);
+ of_clk_unregister_boot_clk(np);
list_add(&cp->link, &of_clk_providers);
mutex_unlock(&of_clk_mutex);
pr_debug("Added clk_hw provider from %pOF\n", np);
@@ -4837,6 +4979,10 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
break;
}
}
+
+ if (hw == ERR_PTR(-EPROBE_DEFER))
+ hw = of_clk_register_boot_clk(clkspec);
+
mutex_unlock(&of_clk_mutex);
return hw;
--
2.30.2
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-18 21:03 ` [RFC] clk: add boot clock support Sebastian Reichel
@ 2021-03-26 1:27 ` Rob Herring
2021-03-26 1:55 ` Saravana Kannan
0 siblings, 1 reply; 28+ messages in thread
From: Rob Herring @ 2021-03-26 1:27 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Michael Turquette, Stephen Boyd, linux-clk, devicetree,
linux-kernel, kernel, saravanak
+Saravana
On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> is provided by an I2C RTC. Specifying this properly results in a
> circular dependency, since the I2C RTC (and thus its clock) cannot
> be initialized without the i.MX6 clock controller being initialized.
>
> With current code the following path is executed when i.MX6 clock
> controller is probed (and ckil clock is specified to be the I2C RTC
> via DT):
>
> 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> 2. of_clk_get_by_name(ccm_node, "ckil");
> 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> 4. of_clk_get_hw(ccm_node, 0, "ckil")
> 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> 7. error is propagated back, i.MX6q clock controller is probe deferred
> 8. I2C controller is never initialized without clock controller
> I2C RTC is never initialized without I2C controller
> CKIL clock is never initialized without I2C RTC
> clock controller is never initialized without CKIL
>
> To fix the circular dependency this registers a dummy clock when
> the RTC clock is tried to be acquired. The dummy clock will later
> be unregistered when the proper clock is registered for the RTC
> DT node. IIUIC clk_core_reparent_orphans() will take care of
> fixing up the clock tree.
>
> NOTE: For now the patch is compile tested only. If this approach
> is the correct one I will do some testing and properly submit this.
> You can find all the details about the hardware in the following
> patchset:
>
> https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> .../bindings/clock/clock-bindings.txt | 7 +
> drivers/clk/clk.c | 146 ++++++++++++++++++
> 2 files changed, 153 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index f2ea53832ac6..66d67ff4aa0f 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> Clock consumer nodes must never directly reference
> the provider's clock-output-names property.
>
> +boot-clock-frequencies: This property is used to specify that a clock is enabled
> + by default with the provided frequency at boot time. This
> + is required to break circular clock dependencies. For clock
> + providers with #clock-cells = 0 this is a single u32
> + with the frequency in Hz. Otherwise it's a list of
> + clock cell specifier + frequency in Hz.
Seems alright to me. I hadn't thought about the aspect of needing to
know the frequency. Other cases probably don't as you only need the
clocks once both components have registered.
Note this could be lost being threaded in the other series.
Rob
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-26 1:27 ` Rob Herring
@ 2021-03-26 1:55 ` Saravana Kannan
2021-03-26 9:52 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Saravana Kannan @ 2021-03-26 1:55 UTC (permalink / raw)
To: Rob Herring
Cc: Sebastian Reichel, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
>
> +Saravana
>
> On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > is provided by an I2C RTC. Specifying this properly results in a
> > circular dependency, since the I2C RTC (and thus its clock) cannot
> > be initialized without the i.MX6 clock controller being initialized.
> >
> > With current code the following path is executed when i.MX6 clock
> > controller is probed (and ckil clock is specified to be the I2C RTC
> > via DT):
> >
> > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > 2. of_clk_get_by_name(ccm_node, "ckil");
> > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > 8. I2C controller is never initialized without clock controller
> > I2C RTC is never initialized without I2C controller
> > CKIL clock is never initialized without I2C RTC
> > clock controller is never initialized without CKIL
> >
> > To fix the circular dependency this registers a dummy clock when
> > the RTC clock is tried to be acquired. The dummy clock will later
> > be unregistered when the proper clock is registered for the RTC
> > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > fixing up the clock tree.
> >
> > NOTE: For now the patch is compile tested only. If this approach
> > is the correct one I will do some testing and properly submit this.
> > You can find all the details about the hardware in the following
> > patchset:
> >
> > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > .../bindings/clock/clock-bindings.txt | 7 +
> > drivers/clk/clk.c | 146 ++++++++++++++++++
> > 2 files changed, 153 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > index f2ea53832ac6..66d67ff4aa0f 100644
> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > Clock consumer nodes must never directly reference
> > the provider's clock-output-names property.
> >
> > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > + by default with the provided frequency at boot time. This
> > + is required to break circular clock dependencies. For clock
> > + providers with #clock-cells = 0 this is a single u32
> > + with the frequency in Hz. Otherwise it's a list of
> > + clock cell specifier + frequency in Hz.
>
> Seems alright to me. I hadn't thought about the aspect of needing to
> know the frequency. Other cases probably don't as you only need the
> clocks once both components have registered.
>
> Note this could be lost being threaded in the other series.
I read this thread and tried to understand it. But my head isn't right
today (lack of sleep) so I couldn't wrap my head around it. I'll look
at it again after the weekend. In the meantime, Sebastian can you
please point me to the DT file and the specific device nodes (names or
line number) where this cycle is present?
Keeping a clock on until all its consumers probe is part of my TODO
list (next item after fw_devlink=on lands). I already have it working
in AOSP, but need to clean it up for upstream. fw_devlink can also
break *some* cycles (not all). So I'm wondering if the kernel will
solve this automatically soon(ish). If it can solve it automatically,
I'd rather not add new DT bindings because it'll make it more work for
fw_devlink.
Thanks,
Saravana
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-26 1:55 ` Saravana Kannan
@ 2021-03-26 9:52 ` Sebastian Reichel
2021-03-29 20:03 ` Saravana Kannan
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-03-26 9:52 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
[-- Attachment #1: Type: text/plain, Size: 6080 bytes --]
Hi Saravana,
On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> >
> > +Saravana
> >
> > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > is provided by an I2C RTC. Specifying this properly results in a
> > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > be initialized without the i.MX6 clock controller being initialized.
> > >
> > > With current code the following path is executed when i.MX6 clock
> > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > via DT):
> > >
> > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > 8. I2C controller is never initialized without clock controller
> > > I2C RTC is never initialized without I2C controller
> > > CKIL clock is never initialized without I2C RTC
> > > clock controller is never initialized without CKIL
> > >
> > > To fix the circular dependency this registers a dummy clock when
> > > the RTC clock is tried to be acquired. The dummy clock will later
> > > be unregistered when the proper clock is registered for the RTC
> > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > fixing up the clock tree.
> > >
> > > NOTE: For now the patch is compile tested only. If this approach
> > > is the correct one I will do some testing and properly submit this.
> > > You can find all the details about the hardware in the following
> > > patchset:
> > >
> > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > >
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > > .../bindings/clock/clock-bindings.txt | 7 +
> > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > 2 files changed, 153 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > Clock consumer nodes must never directly reference
> > > the provider's clock-output-names property.
> > >
> > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > + by default with the provided frequency at boot time. This
> > > + is required to break circular clock dependencies. For clock
> > > + providers with #clock-cells = 0 this is a single u32
> > > + with the frequency in Hz. Otherwise it's a list of
> > > + clock cell specifier + frequency in Hz.
> >
> > Seems alright to me. I hadn't thought about the aspect of needing to
> > know the frequency. Other cases probably don't as you only need the
> > clocks once both components have registered.
> >
> > Note this could be lost being threaded in the other series.
>
> I read this thread and tried to understand it. But my head isn't right
> today (lack of sleep) so I couldn't wrap my head around it. I'll look
> at it again after the weekend. In the meantime, Sebastian can you
> please point me to the DT file and the specific device nodes (names or
> line number) where this cycle is present?
I have not yet sent an updated DT file, but if you look at this
submission:
https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
There is a node
rtc: m41t62@68 { compatible = "st,m41t62"; };
That is an I2C RTC, which provides a 32.768 kHz clock by default
(i.e. after power loss). This clock signal is used to provide the
i.MX6 CKIL:
------------------------------------
&clks {
clocks = <&rtc>;
clock-names = "ckil";
};
------------------------------------
> Keeping a clock on until all its consumers probe is part of my TODO
> list (next item after fw_devlink=on lands). I already have it working
> in AOSP, but need to clean it up for upstream. fw_devlink can also
> break *some* cycles (not all). So I'm wondering if the kernel will
> solve this automatically soon(ish). If it can solve it automatically,
> I'd rather not add new DT bindings because it'll make it more work for
> fw_devlink.
As written above on Congatec QMX6 an I2C RTC provides one of the
SoC's input frequencies. The SoC basically expects that frequency
to be always enabled and this is what it works like before clock
support had been added to the RTC driver.
With the link properly being described the Kernel tries to probe
the SoC's clock controller during early boot. Then it tries to get a
reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
and that returns -EPROBE_DEFER (because the RTC driver has not
yet been probed). Without the clock controller basically none of
the i.MX6 SoC drivers can probe including the I2C driver. Without
the I2C bus being registered, the RTC driver never probes and the
boot process is stuck.
I'm not sure how fw_devlink can help here. I see exactly two
options to solve this:
a) do not describe the link and keep RTC clock enabled somehow.
(my initial patchset)
b) describe the link, but ignore it during boot.
(what I'm trying to do here)
Thanks,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-26 9:52 ` Sebastian Reichel
@ 2021-03-29 20:03 ` Saravana Kannan
2021-03-29 21:53 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Saravana Kannan @ 2021-03-29 20:03 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi Saravana,
>
> On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > +Saravana
> > >
> > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > be initialized without the i.MX6 clock controller being initialized.
> > > >
> > > > With current code the following path is executed when i.MX6 clock
> > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > via DT):
> > > >
> > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > 8. I2C controller is never initialized without clock controller
> > > > I2C RTC is never initialized without I2C controller
> > > > CKIL clock is never initialized without I2C RTC
> > > > clock controller is never initialized without CKIL
> > > >
> > > > To fix the circular dependency this registers a dummy clock when
> > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > be unregistered when the proper clock is registered for the RTC
> > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > fixing up the clock tree.
> > > >
> > > > NOTE: For now the patch is compile tested only. If this approach
> > > > is the correct one I will do some testing and properly submit this.
> > > > You can find all the details about the hardware in the following
> > > > patchset:
> > > >
> > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > >
> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > ---
> > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > 2 files changed, 153 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > Clock consumer nodes must never directly reference
> > > > the provider's clock-output-names property.
> > > >
> > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > + by default with the provided frequency at boot time. This
> > > > + is required to break circular clock dependencies. For clock
> > > > + providers with #clock-cells = 0 this is a single u32
> > > > + with the frequency in Hz. Otherwise it's a list of
> > > > + clock cell specifier + frequency in Hz.
> > >
> > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > know the frequency. Other cases probably don't as you only need the
> > > clocks once both components have registered.
> > >
> > > Note this could be lost being threaded in the other series.
> >
> > I read this thread and tried to understand it. But my head isn't right
> > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > at it again after the weekend. In the meantime, Sebastian can you
> > please point me to the DT file and the specific device nodes (names or
> > line number) where this cycle is present?
>
> I have not yet sent an updated DT file, but if you look at this
> submission:
>
> https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
>
> There is a node
>
> rtc: m41t62@68 { compatible = "st,m41t62"; };
>
> That is an I2C RTC, which provides a 32.768 kHz clock by default
> (i.e. after power loss). This clock signal is used to provide the
> i.MX6 CKIL:
>
> ------------------------------------
> &clks {
> clocks = <&rtc>;
> clock-names = "ckil";
> };
> ------------------------------------
>
> > Keeping a clock on until all its consumers probe is part of my TODO
> > list (next item after fw_devlink=on lands). I already have it working
> > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > break *some* cycles (not all). So I'm wondering if the kernel will
> > solve this automatically soon(ish). If it can solve it automatically,
> > I'd rather not add new DT bindings because it'll make it more work for
> > fw_devlink.
>
> As written above on Congatec QMX6 an I2C RTC provides one of the
> SoC's input frequencies. The SoC basically expects that frequency
> to be always enabled and this is what it works like before clock
> support had been added to the RTC driver.
Thanks. I skimmed through the RTC driver code and
imx6q_obtain_fixed_clk_hw() and the DT files.
>
> With the link properly being described the Kernel tries to probe
> the SoC's clock controller during early boot. Then it tries to get a
> reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> and that returns -EPROBE_DEFER (because the RTC driver has not
> yet been probed).
But the RTC (which is a proper I2C device) will never probe before
CLK_OF_DECLARE() initializes the core clock controller. So, it's not
clear how "protected-clocks" helps here since it doesn't change
whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
is called from the CLK_OF_DECLARE() callback). Oof... I see what you
are doing with of_clk_register_boot_clk(). You are having the consumer
register its own clock and then use it. Kinda beats the whole point of
describing the link in the first place.
> Without the clock controller basically none of
> the i.MX6 SoC drivers can probe including the I2C driver. Without
> the I2C bus being registered, the RTC driver never probes and the
> boot process is stuck.
>
> I'm not sure how fw_devlink can help here.
I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
implemented as an actual platform device driver and not using
CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
assumption later.
In that case, fw_devlink will notice this cycle:
syntax: consumer -(reason)-> supplier
clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
It'll then reason that it doesn't make sense for a device (clks) to
have a supplier (rtc) whose parent (i2c3) in turn depends on the
device (clks). It'll then drop the clks -> rtc dependency because
that's the most illogical one in terms of probing.
So all you'd need to do is delete any -EPROBE defer you might do in
"fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
fw_devlink will make sure the supplier of ckil has probed first. For
cases where there's a cycle like this, it'll be smart enough to drop
this dependency during probe ordering.
I don't know enough about the clocks in imx6q to comment if you can
get away without using CLK_OF_DECLARE() at all. The only clock that
really needs to use CLK_OF_DECLARE() is any clock that's needed for
the scheduler timer. Other than that, everything else can be
initialized by a normal driver. Including UART clocks. I can get into
more specifics if you go down this path.
So, that's how fw_devlink could help here if you massage
drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
have to set fw_devlink=on in the kernel commandline though (it's work
in progress to set this by default). There are some additional details
here about keeping clocks on, but we can discuss the solution for that
if it becomes an issue.
> I see exactly two
> options to solve this:
>
> a) do not describe the link and keep RTC clock enabled somehow.
> (my initial patchset)
> b) describe the link, but ignore it during boot.
> (what I'm trying to do here)
>
Even if you completely ignore fw_devlink, why not just model this
clock as a fixed-clock in DT for this specific machine? It's clearly
expecting the clock to be an always on fixed clock. This will also
remove the need for adding "boot-clock-frequencies" binding.
"fixed-clocks" devices are initialized very early on (they use
CLK_OF_DECLARE too) even without their parents probing (not sure I
agree with this, but this is how it works now).
Something like:
rtc: m41t62@68 {
compatible = "st,m41t62";
reg = <0x68>;
clock-ckil {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <32768>;
};
};
I hope this helps.
-Saravana
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-29 20:03 ` Saravana Kannan
@ 2021-03-29 21:53 ` Sebastian Reichel
2021-03-30 0:36 ` Saravana Kannan
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-03-29 21:53 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
[-- Attachment #1: Type: text/plain, Size: 10840 bytes --]
Hi,
On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote:
> On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > > be initialized without the i.MX6 clock controller being initialized.
> > > > >
> > > > > With current code the following path is executed when i.MX6 clock
> > > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > > via DT):
> > > > >
> > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > > 8. I2C controller is never initialized without clock controller
> > > > > I2C RTC is never initialized without I2C controller
> > > > > CKIL clock is never initialized without I2C RTC
> > > > > clock controller is never initialized without CKIL
> > > > >
> > > > > To fix the circular dependency this registers a dummy clock when
> > > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > > be unregistered when the proper clock is registered for the RTC
> > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > > fixing up the clock tree.
> > > > >
> > > > > NOTE: For now the patch is compile tested only. If this approach
> > > > > is the correct one I will do some testing and properly submit this.
> > > > > You can find all the details about the hardware in the following
> > > > > patchset:
> > > > >
> > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > > >
> > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > ---
> > > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > > 2 files changed, 153 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > > Clock consumer nodes must never directly reference
> > > > > the provider's clock-output-names property.
> > > > >
> > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > > + by default with the provided frequency at boot time. This
> > > > > + is required to break circular clock dependencies. For clock
> > > > > + providers with #clock-cells = 0 this is a single u32
> > > > > + with the frequency in Hz. Otherwise it's a list of
> > > > > + clock cell specifier + frequency in Hz.
> > > >
> > > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > > know the frequency. Other cases probably don't as you only need the
> > > > clocks once both components have registered.
> > > >
> > > > Note this could be lost being threaded in the other series.
> > >
> > > I read this thread and tried to understand it. But my head isn't right
> > > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > > at it again after the weekend. In the meantime, Sebastian can you
> > > please point me to the DT file and the specific device nodes (names or
> > > line number) where this cycle is present?
> >
> > I have not yet sent an updated DT file, but if you look at this
> > submission:
> >
> > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
> >
> > There is a node
> >
> > rtc: m41t62@68 { compatible = "st,m41t62"; };
> >
> > That is an I2C RTC, which provides a 32.768 kHz clock by default
> > (i.e. after power loss). This clock signal is used to provide the
> > i.MX6 CKIL:
> >
> > ------------------------------------
> > &clks {
> > clocks = <&rtc>;
> > clock-names = "ckil";
> > };
> > ------------------------------------
> >
> > > Keeping a clock on until all its consumers probe is part of my TODO
> > > list (next item after fw_devlink=on lands). I already have it working
> > > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > > break *some* cycles (not all). So I'm wondering if the kernel will
> > > solve this automatically soon(ish). If it can solve it automatically,
> > > I'd rather not add new DT bindings because it'll make it more work for
> > > fw_devlink.
> >
> > As written above on Congatec QMX6 an I2C RTC provides one of the
> > SoC's input frequencies. The SoC basically expects that frequency
> > to be always enabled and this is what it works like before clock
> > support had been added to the RTC driver.
>
> Thanks. I skimmed through the RTC driver code and
> imx6q_obtain_fixed_clk_hw() and the DT files.
>
> >
> > With the link properly being described the Kernel tries to probe
> > the SoC's clock controller during early boot. Then it tries to get a
> > reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> > and that returns -EPROBE_DEFER (because the RTC driver has not
> > yet been probed).
>
> But the RTC (which is a proper I2C device) will never probe before
> CLK_OF_DECLARE() initializes the core clock controller. So, it's not
> clear how "protected-clocks" helps here since it doesn't change
> whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
> is called from the CLK_OF_DECLARE() callback). Oof... I see what you
> are doing with of_clk_register_boot_clk(). You are having the consumer
> register its own clock and then use it. Kinda beats the whole point of
> describing the link in the first place.
I agree, that it does not make sense from a code point of view for
this platform. All of this is just to make the DT look correct.
From a platform point of view the most logical way is to handle the
RTC clock as do-not-touch always enabled fixed-clock.
> > Without the clock controller basically none of
> > the i.MX6 SoC drivers can probe including the I2C driver. Without
> > the I2C bus being registered, the RTC driver never probes and the
> > boot process is stuck.
> >
> > I'm not sure how fw_devlink can help here.
>
> I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
> implemented as an actual platform device driver and not using
> CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
> assumption later.
>
> In that case, fw_devlink will notice this cycle:
> syntax: consumer -(reason)-> supplier
> clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
>
> It'll then reason that it doesn't make sense for a device (clks) to
> have a supplier (rtc) whose parent (i2c3) in turn depends on the
> device (clks). It'll then drop the clks -> rtc dependency because
> that's the most illogical one in terms of probing.
>
> So all you'd need to do is delete any -EPROBE defer you might do in
> "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
> fw_devlink will make sure the supplier of ckil has probed first. For
> cases where there's a cycle like this, it'll be smart enough to drop
> this dependency during probe ordering.
What do you mean drop? Anything using ckil will not be registered?
That will basically kill the system within a few seconds, since the
watchdog hardware uses ckil.
> I don't know enough about the clocks in imx6q to comment if you can
> get away without using CLK_OF_DECLARE() at all. The only clock that
> really needs to use CLK_OF_DECLARE() is any clock that's needed for
> the scheduler timer. Other than that, everything else can be
> initialized by a normal driver. Including UART clocks. I can get into
> more specifics if you go down this path.
>
> So, that's how fw_devlink could help here if you massage
> drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
> have to set fw_devlink=on in the kernel commandline though (it's work
> in progress to set this by default). There are some additional details
> here about keeping clocks on, but we can discuss the solution for that
> if it becomes an issue.
>
> > I see exactly two
> > options to solve this:
> >
> > a) do not describe the link and keep RTC clock enabled somehow.
> > (my initial patchset)
> > b) describe the link, but ignore it during boot.
> > (what I'm trying to do here)
> >
>
> Even if you completely ignore fw_devlink, why not just model this
> clock as a fixed-clock in DT for this specific machine?
>
> It's clearly expecting the clock to be an always on fixed clock.
Yes. SoC runs unreliably with this. Downstream vendor kernel does
not contain a clock driver for the squarewave pin of the RTC (i.e.
their driver does not yet contain 1373e77b4f10) and just works.
Upstream kernel disables the RTC's squarewave and then goes into
reboot loop because of watchdog going crazy.
> This will also remove the need for adding "boot-clock-frequencies"
> binding. "fixed-clocks" devices are initialized very early on
> (they use CLK_OF_DECLARE too) even without their parents probing
> (not sure I agree with this, but this is how it works now).
>
> Something like:
>
> rtc: m41t62@68 {
> compatible = "st,m41t62";
> reg = <0x68>;
>
> clock-ckil {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <32768>;
> };
> };
>
> I hope this helps.
This looks like a complex way of my initial patchset with
'protected-clocks' property replaced by a fixed-clock
node. RTC driver needs to check if that exists and avoid
registering its own clock.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-29 21:53 ` Sebastian Reichel
@ 2021-03-30 0:36 ` Saravana Kannan
2021-03-30 9:09 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Saravana Kannan @ 2021-03-30 0:36 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote:
> > On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > > > be initialized without the i.MX6 clock controller being initialized.
> > > > > >
> > > > > > With current code the following path is executed when i.MX6 clock
> > > > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > > > via DT):
> > > > > >
> > > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > > > 8. I2C controller is never initialized without clock controller
> > > > > > I2C RTC is never initialized without I2C controller
> > > > > > CKIL clock is never initialized without I2C RTC
> > > > > > clock controller is never initialized without CKIL
> > > > > >
> > > > > > To fix the circular dependency this registers a dummy clock when
> > > > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > > > be unregistered when the proper clock is registered for the RTC
> > > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > > > fixing up the clock tree.
> > > > > >
> > > > > > NOTE: For now the patch is compile tested only. If this approach
> > > > > > is the correct one I will do some testing and properly submit this.
> > > > > > You can find all the details about the hardware in the following
> > > > > > patchset:
> > > > > >
> > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > > > >
> > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > ---
> > > > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > > > 2 files changed, 153 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > > > Clock consumer nodes must never directly reference
> > > > > > the provider's clock-output-names property.
> > > > > >
> > > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > > > + by default with the provided frequency at boot time. This
> > > > > > + is required to break circular clock dependencies. For clock
> > > > > > + providers with #clock-cells = 0 this is a single u32
> > > > > > + with the frequency in Hz. Otherwise it's a list of
> > > > > > + clock cell specifier + frequency in Hz.
> > > > >
> > > > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > > > know the frequency. Other cases probably don't as you only need the
> > > > > clocks once both components have registered.
> > > > >
> > > > > Note this could be lost being threaded in the other series.
> > > >
> > > > I read this thread and tried to understand it. But my head isn't right
> > > > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > > > at it again after the weekend. In the meantime, Sebastian can you
> > > > please point me to the DT file and the specific device nodes (names or
> > > > line number) where this cycle is present?
> > >
> > > I have not yet sent an updated DT file, but if you look at this
> > > submission:
> > >
> > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
> > >
> > > There is a node
> > >
> > > rtc: m41t62@68 { compatible = "st,m41t62"; };
> > >
> > > That is an I2C RTC, which provides a 32.768 kHz clock by default
> > > (i.e. after power loss). This clock signal is used to provide the
> > > i.MX6 CKIL:
> > >
> > > ------------------------------------
> > > &clks {
> > > clocks = <&rtc>;
> > > clock-names = "ckil";
> > > };
> > > ------------------------------------
> > >
> > > > Keeping a clock on until all its consumers probe is part of my TODO
> > > > list (next item after fw_devlink=on lands). I already have it working
> > > > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > > > break *some* cycles (not all). So I'm wondering if the kernel will
> > > > solve this automatically soon(ish). If it can solve it automatically,
> > > > I'd rather not add new DT bindings because it'll make it more work for
> > > > fw_devlink.
> > >
> > > As written above on Congatec QMX6 an I2C RTC provides one of the
> > > SoC's input frequencies. The SoC basically expects that frequency
> > > to be always enabled and this is what it works like before clock
> > > support had been added to the RTC driver.
> >
> > Thanks. I skimmed through the RTC driver code and
> > imx6q_obtain_fixed_clk_hw() and the DT files.
> >
> > >
> > > With the link properly being described the Kernel tries to probe
> > > the SoC's clock controller during early boot. Then it tries to get a
> > > reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> > > and that returns -EPROBE_DEFER (because the RTC driver has not
> > > yet been probed).
> >
> > But the RTC (which is a proper I2C device) will never probe before
> > CLK_OF_DECLARE() initializes the core clock controller. So, it's not
> > clear how "protected-clocks" helps here since it doesn't change
> > whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
> > is called from the CLK_OF_DECLARE() callback). Oof... I see what you
> > are doing with of_clk_register_boot_clk(). You are having the consumer
> > register its own clock and then use it. Kinda beats the whole point of
> > describing the link in the first place.
>
> I agree, that it does not make sense from a code point of view for
> this platform. All of this is just to make the DT look correct.
> From a platform point of view the most logical way is to handle the
> RTC clock as do-not-touch always enabled fixed-clock.
>
> > > Without the clock controller basically none of
> > > the i.MX6 SoC drivers can probe including the I2C driver. Without
> > > the I2C bus being registered, the RTC driver never probes and the
> > > boot process is stuck.
> > >
> > > I'm not sure how fw_devlink can help here.
> >
> > I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
> > implemented as an actual platform device driver and not using
> > CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
> > assumption later.
> >
> > In that case, fw_devlink will notice this cycle:
> > syntax: consumer -(reason)-> supplier
> > clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
> >
> > It'll then reason that it doesn't make sense for a device (clks) to
> > have a supplier (rtc) whose parent (i2c3) in turn depends on the
> > device (clks). It'll then drop the clks -> rtc dependency because
> > that's the most illogical one in terms of probing.
> >
> > So all you'd need to do is delete any -EPROBE defer you might do in
> > "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
> > fw_devlink will make sure the supplier of ckil has probed first. For
> > cases where there's a cycle like this, it'll be smart enough to drop
> > this dependency during probe ordering.
>
> What do you mean drop? Anything using ckil will not be registered?
> That will basically kill the system within a few seconds, since the
> watchdog hardware uses ckil.
No, it means that it won't block CCM on ckil. It's not a generic
"ignore dependency for all consumers of ckil". fw_devlink does this
specifically to the link that causes a probe dependency cycle.
> > I don't know enough about the clocks in imx6q to comment if you can
> > get away without using CLK_OF_DECLARE() at all. The only clock that
> > really needs to use CLK_OF_DECLARE() is any clock that's needed for
> > the scheduler timer. Other than that, everything else can be
> > initialized by a normal driver. Including UART clocks. I can get into
> > more specifics if you go down this path.
> >
> > So, that's how fw_devlink could help here if you massage
> > drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
> > have to set fw_devlink=on in the kernel commandline though (it's work
> > in progress to set this by default). There are some additional details
> > here about keeping clocks on, but we can discuss the solution for that
> > if it becomes an issue.
> >
> > > I see exactly two
> > > options to solve this:
> > >
> > > a) do not describe the link and keep RTC clock enabled somehow.
> > > (my initial patchset)
> > > b) describe the link, but ignore it during boot.
> > > (what I'm trying to do here)
> > >
> >
> > Even if you completely ignore fw_devlink, why not just model this
> > clock as a fixed-clock in DT for this specific machine?
> >
> > It's clearly expecting the clock to be an always on fixed clock.
>
> Yes. SoC runs unreliably with this. Downstream vendor kernel does
> not contain a clock driver for the squarewave pin of the RTC (i.e.
> their driver does not yet contain 1373e77b4f10) and just works.
> Upstream kernel disables the RTC's squarewave and then goes into
> reboot loop because of watchdog going crazy.
>
> > This will also remove the need for adding "boot-clock-frequencies"
> > binding. "fixed-clocks" devices are initialized very early on
> > (they use CLK_OF_DECLARE too) even without their parents probing
> > (not sure I agree with this, but this is how it works now).
> >
> > Something like:
> >
> > rtc: m41t62@68 {
> > compatible = "st,m41t62";
> > reg = <0x68>;
> >
> > clock-ckil {
> > compatible = "fixed-clock";
> > #clock-cells = <0>;
> > clock-frequency = <32768>;
> > };
> > };
> >
> > I hope this helps.
>
> This looks like a complex way of my initial patchset with
> 'protected-clocks' property replaced by a fixed-clock
> node. RTC driver needs to check if that exists and avoid
> registering its own clock.
If anything, I'd argue this is a lot more simpler because it avoids
adding a new DT binding, it avoids changes to drivers/clk/clk.c.
Instead of checking for "protected-clocks" you just check for this
child node (or just any child node). Also, technically if you set the
CLK_IGNORE_UNUSED flag for the clock, you don't even need to do any
explicit checking in the RTC driver as long as some other driver
doesn't try to get this clock and turn it on/off.
-Saravana
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-30 0:36 ` Saravana Kannan
@ 2021-03-30 9:09 ` Sebastian Reichel
2021-03-30 17:05 ` Saravana Kannan
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-03-30 9:09 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
[-- Attachment #1: Type: text/plain, Size: 13395 bytes --]
Hi,
On Mon, Mar 29, 2021 at 05:36:11PM -0700, Saravana Kannan wrote:
> On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> > On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote:
> > > On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
> > > <sebastian.reichel@collabora.com> wrote:
> > > > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > > > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > > > > be initialized without the i.MX6 clock controller being initialized.
> > > > > > >
> > > > > > > With current code the following path is executed when i.MX6 clock
> > > > > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > > > > via DT):
> > > > > > >
> > > > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > > > > 8. I2C controller is never initialized without clock controller
> > > > > > > I2C RTC is never initialized without I2C controller
> > > > > > > CKIL clock is never initialized without I2C RTC
> > > > > > > clock controller is never initialized without CKIL
> > > > > > >
> > > > > > > To fix the circular dependency this registers a dummy clock when
> > > > > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > > > > be unregistered when the proper clock is registered for the RTC
> > > > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > > > > fixing up the clock tree.
> > > > > > >
> > > > > > > NOTE: For now the patch is compile tested only. If this approach
> > > > > > > is the correct one I will do some testing and properly submit this.
> > > > > > > You can find all the details about the hardware in the following
> > > > > > > patchset:
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > > > > >
> > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > ---
> > > > > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > > > > 2 files changed, 153 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > > > > Clock consumer nodes must never directly reference
> > > > > > > the provider's clock-output-names property.
> > > > > > >
> > > > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > > > > + by default with the provided frequency at boot time. This
> > > > > > > + is required to break circular clock dependencies. For clock
> > > > > > > + providers with #clock-cells = 0 this is a single u32
> > > > > > > + with the frequency in Hz. Otherwise it's a list of
> > > > > > > + clock cell specifier + frequency in Hz.
> > > > > >
> > > > > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > > > > know the frequency. Other cases probably don't as you only need the
> > > > > > clocks once both components have registered.
> > > > > >
> > > > > > Note this could be lost being threaded in the other series.
> > > > >
> > > > > I read this thread and tried to understand it. But my head isn't right
> > > > > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > > > > at it again after the weekend. In the meantime, Sebastian can you
> > > > > please point me to the DT file and the specific device nodes (names or
> > > > > line number) where this cycle is present?
> > > >
> > > > I have not yet sent an updated DT file, but if you look at this
> > > > submission:
> > > >
> > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
> > > >
> > > > There is a node
> > > >
> > > > rtc: m41t62@68 { compatible = "st,m41t62"; };
> > > >
> > > > That is an I2C RTC, which provides a 32.768 kHz clock by default
> > > > (i.e. after power loss). This clock signal is used to provide the
> > > > i.MX6 CKIL:
> > > >
> > > > ------------------------------------
> > > > &clks {
> > > > clocks = <&rtc>;
> > > > clock-names = "ckil";
> > > > };
> > > > ------------------------------------
> > > >
> > > > > Keeping a clock on until all its consumers probe is part of my TODO
> > > > > list (next item after fw_devlink=on lands). I already have it working
> > > > > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > > > > break *some* cycles (not all). So I'm wondering if the kernel will
> > > > > solve this automatically soon(ish). If it can solve it automatically,
> > > > > I'd rather not add new DT bindings because it'll make it more work for
> > > > > fw_devlink.
> > > >
> > > > As written above on Congatec QMX6 an I2C RTC provides one of the
> > > > SoC's input frequencies. The SoC basically expects that frequency
> > > > to be always enabled and this is what it works like before clock
> > > > support had been added to the RTC driver.
> > >
> > > Thanks. I skimmed through the RTC driver code and
> > > imx6q_obtain_fixed_clk_hw() and the DT files.
> > >
> > > >
> > > > With the link properly being described the Kernel tries to probe
> > > > the SoC's clock controller during early boot. Then it tries to get a
> > > > reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> > > > and that returns -EPROBE_DEFER (because the RTC driver has not
> > > > yet been probed).
> > >
> > > But the RTC (which is a proper I2C device) will never probe before
> > > CLK_OF_DECLARE() initializes the core clock controller. So, it's not
> > > clear how "protected-clocks" helps here since it doesn't change
> > > whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
> > > is called from the CLK_OF_DECLARE() callback). Oof... I see what you
> > > are doing with of_clk_register_boot_clk(). You are having the consumer
> > > register its own clock and then use it. Kinda beats the whole point of
> > > describing the link in the first place.
> >
> > I agree, that it does not make sense from a code point of view for
> > this platform. All of this is just to make the DT look correct.
> > From a platform point of view the most logical way is to handle the
> > RTC clock as do-not-touch always enabled fixed-clock.
> >
> > > > Without the clock controller basically none of
> > > > the i.MX6 SoC drivers can probe including the I2C driver. Without
> > > > the I2C bus being registered, the RTC driver never probes and the
> > > > boot process is stuck.
> > > >
> > > > I'm not sure how fw_devlink can help here.
> > >
> > > I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
> > > implemented as an actual platform device driver and not using
> > > CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
> > > assumption later.
> > >
> > > In that case, fw_devlink will notice this cycle:
> > > syntax: consumer -(reason)-> supplier
> > > clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
> > >
> > > It'll then reason that it doesn't make sense for a device (clks) to
> > > have a supplier (rtc) whose parent (i2c3) in turn depends on the
> > > device (clks). It'll then drop the clks -> rtc dependency because
> > > that's the most illogical one in terms of probing.
> > >
> > > So all you'd need to do is delete any -EPROBE defer you might do in
> > > "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
> > > fw_devlink will make sure the supplier of ckil has probed first. For
> > > cases where there's a cycle like this, it'll be smart enough to drop
> > > this dependency during probe ordering.
> >
> > What do you mean drop? Anything using ckil will not be registered?
> > That will basically kill the system within a few seconds, since the
> > watchdog hardware uses ckil.
>
> No, it means that it won't block CCM on ckil. It's not a generic
> "ignore dependency for all consumers of ckil". fw_devlink does this
> specifically to the link that causes a probe dependency cycle.
I still don't follow. If CCM proceeds booting without blocking on
missing CCM, what would be the content of hws[IMX6QDL_CLK_CKIL]?
What ensures, that the consumers get correct clock rates?
> > > I don't know enough about the clocks in imx6q to comment if you can
> > > get away without using CLK_OF_DECLARE() at all. The only clock that
> > > really needs to use CLK_OF_DECLARE() is any clock that's needed for
> > > the scheduler timer. Other than that, everything else can be
> > > initialized by a normal driver. Including UART clocks. I can get into
> > > more specifics if you go down this path.
> > >
> > > So, that's how fw_devlink could help here if you massage
> > > drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
> > > have to set fw_devlink=on in the kernel commandline though (it's work
> > > in progress to set this by default). There are some additional details
> > > here about keeping clocks on, but we can discuss the solution for that
> > > if it becomes an issue.
> > >
> > > > I see exactly two
> > > > options to solve this:
> > > >
> > > > a) do not describe the link and keep RTC clock enabled somehow.
> > > > (my initial patchset)
> > > > b) describe the link, but ignore it during boot.
> > > > (what I'm trying to do here)
> > > >
> > >
> > > Even if you completely ignore fw_devlink, why not just model this
> > > clock as a fixed-clock in DT for this specific machine?
> > >
> > > It's clearly expecting the clock to be an always on fixed clock.
> >
> > Yes. SoC runs unreliably with this. Downstream vendor kernel does
> > not contain a clock driver for the squarewave pin of the RTC (i.e.
> > their driver does not yet contain 1373e77b4f10) and just works.
> > Upstream kernel disables the RTC's squarewave and then goes into
> > reboot loop because of watchdog going crazy.
> >
> > > This will also remove the need for adding "boot-clock-frequencies"
> > > binding. "fixed-clocks" devices are initialized very early on
> > > (they use CLK_OF_DECLARE too) even without their parents probing
> > > (not sure I agree with this, but this is how it works now).
> > >
> > > Something like:
> > >
> > > rtc: m41t62@68 {
> > > compatible = "st,m41t62";
> > > reg = <0x68>;
> > >
> > > clock-ckil {
> > > compatible = "fixed-clock";
> > > #clock-cells = <0>;
> > > clock-frequency = <32768>;
> > > };
> > > };
> > >
> > > I hope this helps.
> >
> > This looks like a complex way of my initial patchset with
> > 'protected-clocks' property replaced by a fixed-clock
> > node. RTC driver needs to check if that exists and avoid
> > registering its own clock.
>
> If anything, I'd argue this is a lot more simpler because it avoids
> adding a new DT binding, it avoids changes to drivers/clk/clk.c.
My original patch [0] is a two liner, which does not change
drivers/clk/clk.c and protected-clocks is a standard property
from [1]. I think you confused this with the boot-clock-frequencies
approach :)
[0] https://lore.kernel.org/linux-devicetree/20210222171247.97609-2-sebastian.reichel@collabora.com/
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt.
> Instead of checking for "protected-clocks" you just check for this
> child node (or just any child node). Also, technically if you set the
> CLK_IGNORE_UNUSED flag for the clock, you don't even need to do any
> explicit checking in the RTC driver as long as some other driver
> doesn't try to get this clock and turn it on/off.
Child nodes are part of DT binding, so the information about the
potential clock subnode also needs to be added to the RTC binding.
It also changes the reference point from referencing the RTC node
to referencing a subnode, which seems a bit inconsistent to me.
Thanks,
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-30 9:09 ` Sebastian Reichel
@ 2021-03-30 17:05 ` Saravana Kannan
2021-04-05 22:43 ` Sebastian Reichel
0 siblings, 1 reply; 28+ messages in thread
From: Saravana Kannan @ 2021-03-30 17:05 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
On Tue, Mar 30, 2021 at 2:09 AM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Mon, Mar 29, 2021 at 05:36:11PM -0700, Saravana Kannan wrote:
> > On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > > On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote:
> > > > On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
> > > > <sebastian.reichel@collabora.com> wrote:
> > > > > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > > > > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > > > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > > > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > > > > > be initialized without the i.MX6 clock controller being initialized.
> > > > > > > >
> > > > > > > > With current code the following path is executed when i.MX6 clock
> > > > > > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > > > > > via DT):
> > > > > > > >
> > > > > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > > > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > > > > > 8. I2C controller is never initialized without clock controller
> > > > > > > > I2C RTC is never initialized without I2C controller
> > > > > > > > CKIL clock is never initialized without I2C RTC
> > > > > > > > clock controller is never initialized without CKIL
> > > > > > > >
> > > > > > > > To fix the circular dependency this registers a dummy clock when
> > > > > > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > > > > > be unregistered when the proper clock is registered for the RTC
> > > > > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > > > > > fixing up the clock tree.
> > > > > > > >
> > > > > > > > NOTE: For now the patch is compile tested only. If this approach
> > > > > > > > is the correct one I will do some testing and properly submit this.
> > > > > > > > You can find all the details about the hardware in the following
> > > > > > > > patchset:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > > > > > >
> > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > > ---
> > > > > > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > > > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > > > > > 2 files changed, 153 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > > > > > Clock consumer nodes must never directly reference
> > > > > > > > the provider's clock-output-names property.
> > > > > > > >
> > > > > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > > > > > + by default with the provided frequency at boot time. This
> > > > > > > > + is required to break circular clock dependencies. For clock
> > > > > > > > + providers with #clock-cells = 0 this is a single u32
> > > > > > > > + with the frequency in Hz. Otherwise it's a list of
> > > > > > > > + clock cell specifier + frequency in Hz.
> > > > > > >
> > > > > > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > > > > > know the frequency. Other cases probably don't as you only need the
> > > > > > > clocks once both components have registered.
> > > > > > >
> > > > > > > Note this could be lost being threaded in the other series.
> > > > > >
> > > > > > I read this thread and tried to understand it. But my head isn't right
> > > > > > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > > > > > at it again after the weekend. In the meantime, Sebastian can you
> > > > > > please point me to the DT file and the specific device nodes (names or
> > > > > > line number) where this cycle is present?
> > > > >
> > > > > I have not yet sent an updated DT file, but if you look at this
> > > > > submission:
> > > > >
> > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
> > > > >
> > > > > There is a node
> > > > >
> > > > > rtc: m41t62@68 { compatible = "st,m41t62"; };
> > > > >
> > > > > That is an I2C RTC, which provides a 32.768 kHz clock by default
> > > > > (i.e. after power loss). This clock signal is used to provide the
> > > > > i.MX6 CKIL:
> > > > >
> > > > > ------------------------------------
> > > > > &clks {
> > > > > clocks = <&rtc>;
> > > > > clock-names = "ckil";
> > > > > };
> > > > > ------------------------------------
> > > > >
> > > > > > Keeping a clock on until all its consumers probe is part of my TODO
> > > > > > list (next item after fw_devlink=on lands). I already have it working
> > > > > > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > > > > > break *some* cycles (not all). So I'm wondering if the kernel will
> > > > > > solve this automatically soon(ish). If it can solve it automatically,
> > > > > > I'd rather not add new DT bindings because it'll make it more work for
> > > > > > fw_devlink.
> > > > >
> > > > > As written above on Congatec QMX6 an I2C RTC provides one of the
> > > > > SoC's input frequencies. The SoC basically expects that frequency
> > > > > to be always enabled and this is what it works like before clock
> > > > > support had been added to the RTC driver.
> > > >
> > > > Thanks. I skimmed through the RTC driver code and
> > > > imx6q_obtain_fixed_clk_hw() and the DT files.
> > > >
> > > > >
> > > > > With the link properly being described the Kernel tries to probe
> > > > > the SoC's clock controller during early boot. Then it tries to get a
> > > > > reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> > > > > and that returns -EPROBE_DEFER (because the RTC driver has not
> > > > > yet been probed).
> > > >
> > > > But the RTC (which is a proper I2C device) will never probe before
> > > > CLK_OF_DECLARE() initializes the core clock controller. So, it's not
> > > > clear how "protected-clocks" helps here since it doesn't change
> > > > whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
> > > > is called from the CLK_OF_DECLARE() callback). Oof... I see what you
> > > > are doing with of_clk_register_boot_clk(). You are having the consumer
> > > > register its own clock and then use it. Kinda beats the whole point of
> > > > describing the link in the first place.
> > >
> > > I agree, that it does not make sense from a code point of view for
> > > this platform. All of this is just to make the DT look correct.
> > > From a platform point of view the most logical way is to handle the
> > > RTC clock as do-not-touch always enabled fixed-clock.
> > >
> > > > > Without the clock controller basically none of
> > > > > the i.MX6 SoC drivers can probe including the I2C driver. Without
> > > > > the I2C bus being registered, the RTC driver never probes and the
> > > > > boot process is stuck.
> > > > >
> > > > > I'm not sure how fw_devlink can help here.
> > > >
> > > > I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
> > > > implemented as an actual platform device driver and not using
> > > > CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
> > > > assumption later.
> > > >
> > > > In that case, fw_devlink will notice this cycle:
> > > > syntax: consumer -(reason)-> supplier
> > > > clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
> > > >
> > > > It'll then reason that it doesn't make sense for a device (clks) to
> > > > have a supplier (rtc) whose parent (i2c3) in turn depends on the
> > > > device (clks). It'll then drop the clks -> rtc dependency because
> > > > that's the most illogical one in terms of probing.
> > > >
> > > > So all you'd need to do is delete any -EPROBE defer you might do in
> > > > "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
> > > > fw_devlink will make sure the supplier of ckil has probed first. For
> > > > cases where there's a cycle like this, it'll be smart enough to drop
> > > > this dependency during probe ordering.
> > >
> > > What do you mean drop? Anything using ckil will not be registered?
> > > That will basically kill the system within a few seconds, since the
> > > watchdog hardware uses ckil.
> >
> > No, it means that it won't block CCM on ckil. It's not a generic
> > "ignore dependency for all consumers of ckil". fw_devlink does this
> > specifically to the link that causes a probe dependency cycle.
>
> I still don't follow. If CCM proceeds booting without blocking on
> missing CCM,
I think you meant to say missing CKIL,
> what would be the content of hws[IMX6QDL_CLK_CKIL]?
> What ensures, that the consumers get correct clock rates?
I haven't dug into the IMX CCM driver, but my current understanding is
that the 32 KHz clock is the CKIL input coming into CCM and it's a
parent/ancestor to some/all of the CCM clocks. The clock framework
allows you to register clocks before their parents are registered
(because clocks are messy and clock providers can have cycles between
them). So if the IMX CCM driver is written correctly, it'd register
the clock with the clock framework saying "hey, my parent is clock
CKIL from this other DT node, connect us up when it's registered".
I'll let you figure out the details of the implementation.
Also, as I said before, the fixed-clock(s) will be available and
working before the RTC probes. So, either the CCM registers first or
the CKIL fixed-clock registers first and the same caller would
register the other. And then the clock framework will connect them up
and everything will continue working nicely.
> > > > I don't know enough about the clocks in imx6q to comment if you can
> > > > get away without using CLK_OF_DECLARE() at all. The only clock that
> > > > really needs to use CLK_OF_DECLARE() is any clock that's needed for
> > > > the scheduler timer. Other than that, everything else can be
> > > > initialized by a normal driver. Including UART clocks. I can get into
> > > > more specifics if you go down this path.
> > > >
> > > > So, that's how fw_devlink could help here if you massage
> > > > drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
> > > > have to set fw_devlink=on in the kernel commandline though (it's work
> > > > in progress to set this by default). There are some additional details
> > > > here about keeping clocks on, but we can discuss the solution for that
> > > > if it becomes an issue.
> > > >
> > > > > I see exactly two
> > > > > options to solve this:
> > > > >
> > > > > a) do not describe the link and keep RTC clock enabled somehow.
> > > > > (my initial patchset)
> > > > > b) describe the link, but ignore it during boot.
> > > > > (what I'm trying to do here)
> > > > >
> > > >
> > > > Even if you completely ignore fw_devlink, why not just model this
> > > > clock as a fixed-clock in DT for this specific machine?
> > > >
> > > > It's clearly expecting the clock to be an always on fixed clock.
> > >
> > > Yes. SoC runs unreliably with this. Downstream vendor kernel does
> > > not contain a clock driver for the squarewave pin of the RTC (i.e.
> > > their driver does not yet contain 1373e77b4f10) and just works.
> > > Upstream kernel disables the RTC's squarewave and then goes into
> > > reboot loop because of watchdog going crazy.
> > >
> > > > This will also remove the need for adding "boot-clock-frequencies"
> > > > binding. "fixed-clocks" devices are initialized very early on
> > > > (they use CLK_OF_DECLARE too) even without their parents probing
> > > > (not sure I agree with this, but this is how it works now).
> > > >
> > > > Something like:
> > > >
> > > > rtc: m41t62@68 {
> > > > compatible = "st,m41t62";
> > > > reg = <0x68>;
> > > >
> > > > clock-ckil {
> > > > compatible = "fixed-clock";
> > > > #clock-cells = <0>;
> > > > clock-frequency = <32768>;
> > > > };
> > > > };
> > > >
> > > > I hope this helps.
> > >
> > > This looks like a complex way of my initial patchset with
> > > 'protected-clocks' property replaced by a fixed-clock
> > > node. RTC driver needs to check if that exists and avoid
> > > registering its own clock.
> >
> > If anything, I'd argue this is a lot more simpler because it avoids
> > adding a new DT binding, it avoids changes to drivers/clk/clk.c.
>
> My original patch [0] is a two liner, which does not change
> drivers/clk/clk.c and protected-clocks is a standard property
> from [1]. I think you confused this with the boot-clock-frequencies
> approach :)
I think my confusion was that you wanted to do both [0] and [1]
because of them being threaded and not having v1/v2.
Just to clarify, I'm not NAKing any patch here. I'm just explaining
how things work and giving options because I was CCed. I'll leave it
to Stephen/Rob to decide what they want to accept.
But I can see the point in Rob's request for wanting the DT to capture
the real hardware connections correctly.
> [0] https://lore.kernel.org/linux-devicetree/20210222171247.97609-2-sebastian.reichel@collabora.com/
> [1] Documentation/devicetree/bindings/clock/clock-bindings.txt.
>
> > Instead of checking for "protected-clocks" you just check for this
> > child node (or just any child node). Also, technically if you set the
> > CLK_IGNORE_UNUSED flag for the clock, you don't even need to do any
> > explicit checking in the RTC driver as long as some other driver
> > doesn't try to get this clock and turn it on/off.
>
> Child nodes are part of DT binding, so the information about the
> potential clock subnode also needs to be added to the RTC binding.
> It also changes the reference point from referencing the RTC node
> to referencing a subnode, which seems a bit inconsistent to me.
Sure, you can add a child node to the RTC binding. But it's not a new
DT property binding (if you go with option 2).
-Saravana
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-03-30 17:05 ` Saravana Kannan
@ 2021-04-05 22:43 ` Sebastian Reichel
2021-04-05 23:51 ` Saravana Kannan
0 siblings, 1 reply; 28+ messages in thread
From: Sebastian Reichel @ 2021-04-05 22:43 UTC (permalink / raw)
To: Saravana Kannan
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
[-- Attachment #1: Type: text/plain, Size: 17123 bytes --]
Hi,
On Tue, Mar 30, 2021 at 10:05:45AM -0700, Saravana Kannan wrote:
> On Tue, Mar 30, 2021 at 2:09 AM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> > On Mon, Mar 29, 2021 at 05:36:11PM -0700, Saravana Kannan wrote:
> > > On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel
> > > <sebastian.reichel@collabora.com> wrote:
> > > > On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote:
> > > > > On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
> > > > > <sebastian.reichel@collabora.com> wrote:
> > > > > > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > > > > > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > > > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > > > > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > > > > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > > > > > > be initialized without the i.MX6 clock controller being initialized.
> > > > > > > > >
> > > > > > > > > With current code the following path is executed when i.MX6 clock
> > > > > > > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > > > > > > via DT):
> > > > > > > > >
> > > > > > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > > > > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > > > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > > > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > > > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > > > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > > > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > > > > > > 8. I2C controller is never initialized without clock controller
> > > > > > > > > I2C RTC is never initialized without I2C controller
> > > > > > > > > CKIL clock is never initialized without I2C RTC
> > > > > > > > > clock controller is never initialized without CKIL
> > > > > > > > >
> > > > > > > > > To fix the circular dependency this registers a dummy clock when
> > > > > > > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > > > > > > be unregistered when the proper clock is registered for the RTC
> > > > > > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > > > > > > fixing up the clock tree.
> > > > > > > > >
> > > > > > > > > NOTE: For now the patch is compile tested only. If this approach
> > > > > > > > > is the correct one I will do some testing and properly submit this.
> > > > > > > > > You can find all the details about the hardware in the following
> > > > > > > > > patchset:
> > > > > > > > >
> > > > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > > > > > > >
> > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > > > ---
> > > > > > > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > > > > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > > > > > > 2 files changed, 153 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > > > > > > Clock consumer nodes must never directly reference
> > > > > > > > > the provider's clock-output-names property.
> > > > > > > > >
> > > > > > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > > > > > > + by default with the provided frequency at boot time. This
> > > > > > > > > + is required to break circular clock dependencies. For clock
> > > > > > > > > + providers with #clock-cells = 0 this is a single u32
> > > > > > > > > + with the frequency in Hz. Otherwise it's a list of
> > > > > > > > > + clock cell specifier + frequency in Hz.
> > > > > > > >
> > > > > > > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > > > > > > know the frequency. Other cases probably don't as you only need the
> > > > > > > > clocks once both components have registered.
> > > > > > > >
> > > > > > > > Note this could be lost being threaded in the other series.
> > > > > > >
> > > > > > > I read this thread and tried to understand it. But my head isn't right
> > > > > > > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > > > > > > at it again after the weekend. In the meantime, Sebastian can you
> > > > > > > please point me to the DT file and the specific device nodes (names or
> > > > > > > line number) where this cycle is present?
> > > > > >
> > > > > > I have not yet sent an updated DT file, but if you look at this
> > > > > > submission:
> > > > > >
> > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
> > > > > >
> > > > > > There is a node
> > > > > >
> > > > > > rtc: m41t62@68 { compatible = "st,m41t62"; };
> > > > > >
> > > > > > That is an I2C RTC, which provides a 32.768 kHz clock by default
> > > > > > (i.e. after power loss). This clock signal is used to provide the
> > > > > > i.MX6 CKIL:
> > > > > >
> > > > > > ------------------------------------
> > > > > > &clks {
> > > > > > clocks = <&rtc>;
> > > > > > clock-names = "ckil";
> > > > > > };
> > > > > > ------------------------------------
> > > > > >
> > > > > > > Keeping a clock on until all its consumers probe is part of my TODO
> > > > > > > list (next item after fw_devlink=on lands). I already have it working
> > > > > > > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > > > > > > break *some* cycles (not all). So I'm wondering if the kernel will
> > > > > > > solve this automatically soon(ish). If it can solve it automatically,
> > > > > > > I'd rather not add new DT bindings because it'll make it more work for
> > > > > > > fw_devlink.
> > > > > >
> > > > > > As written above on Congatec QMX6 an I2C RTC provides one of the
> > > > > > SoC's input frequencies. The SoC basically expects that frequency
> > > > > > to be always enabled and this is what it works like before clock
> > > > > > support had been added to the RTC driver.
> > > > >
> > > > > Thanks. I skimmed through the RTC driver code and
> > > > > imx6q_obtain_fixed_clk_hw() and the DT files.
> > > > >
> > > > > >
> > > > > > With the link properly being described the Kernel tries to probe
> > > > > > the SoC's clock controller during early boot. Then it tries to get a
> > > > > > reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> > > > > > and that returns -EPROBE_DEFER (because the RTC driver has not
> > > > > > yet been probed).
> > > > >
> > > > > But the RTC (which is a proper I2C device) will never probe before
> > > > > CLK_OF_DECLARE() initializes the core clock controller. So, it's not
> > > > > clear how "protected-clocks" helps here since it doesn't change
> > > > > whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
> > > > > is called from the CLK_OF_DECLARE() callback). Oof... I see what you
> > > > > are doing with of_clk_register_boot_clk(). You are having the consumer
> > > > > register its own clock and then use it. Kinda beats the whole point of
> > > > > describing the link in the first place.
> > > >
> > > > I agree, that it does not make sense from a code point of view for
> > > > this platform. All of this is just to make the DT look correct.
> > > > From a platform point of view the most logical way is to handle the
> > > > RTC clock as do-not-touch always enabled fixed-clock.
> > > >
> > > > > > Without the clock controller basically none of
> > > > > > the i.MX6 SoC drivers can probe including the I2C driver. Without
> > > > > > the I2C bus being registered, the RTC driver never probes and the
> > > > > > boot process is stuck.
> > > > > >
> > > > > > I'm not sure how fw_devlink can help here.
> > > > >
> > > > > I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
> > > > > implemented as an actual platform device driver and not using
> > > > > CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
> > > > > assumption later.
> > > > >
> > > > > In that case, fw_devlink will notice this cycle:
> > > > > syntax: consumer -(reason)-> supplier
> > > > > clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
> > > > >
> > > > > It'll then reason that it doesn't make sense for a device (clks) to
> > > > > have a supplier (rtc) whose parent (i2c3) in turn depends on the
> > > > > device (clks). It'll then drop the clks -> rtc dependency because
> > > > > that's the most illogical one in terms of probing.
> > > > >
> > > > > So all you'd need to do is delete any -EPROBE defer you might do in
> > > > > "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
> > > > > fw_devlink will make sure the supplier of ckil has probed first. For
> > > > > cases where there's a cycle like this, it'll be smart enough to drop
> > > > > this dependency during probe ordering.
> > > >
> > > > What do you mean drop? Anything using ckil will not be registered?
> > > > That will basically kill the system within a few seconds, since the
> > > > watchdog hardware uses ckil.
> > >
> > > No, it means that it won't block CCM on ckil. It's not a generic
> > > "ignore dependency for all consumers of ckil". fw_devlink does this
> > > specifically to the link that causes a probe dependency cycle.
> >
> > I still don't follow. If CCM proceeds booting without blocking on
> > missing CCM,
>
> I think you meant to say missing CKIL,
Yes.
> > what would be the content of hws[IMX6QDL_CLK_CKIL]?
> > What ensures, that the consumers get correct clock rates?
>
> I haven't dug into the IMX CCM driver, but my current understanding is
> that the 32 KHz clock is the CKIL input coming into CCM and it's a
> parent/ancestor to some/all of the CCM clocks.
Right.
> The clock framework allows you to register clocks before their
> parents are registered (because clocks are messy and clock
> providers can have cycles between them). So if the IMX CCM driver
> is written correctly, it'd register the clock with the clock
> framework saying "hey, my parent is clock CKIL from this other DT
> node, connect us up when it's registered". I'll let you figure
> out the details of the implementation.
I've been told this is supposed to work in theory before, but nobody
could point at an example. All drivers, that I checked end up with
-EPROBE_DEFER on missing parent clocks, which is good enough for
most dependency issues, but not for cyclic dependencies.
> Also, as I said before, the fixed-clock(s) will be available and
> working before the RTC probes. So, either the CCM registers first or
> the CKIL fixed-clock registers first and the same caller would
> register the other. And then the clock framework will connect them up
> and everything will continue working nicely.
Yes, since fixed-clock is completley unrelated to RTC. Without
further driver changes the RTC would still register a clock
device and disable the clock because of it being unused.
> > > > > I don't know enough about the clocks in imx6q to comment if you can
> > > > > get away without using CLK_OF_DECLARE() at all. The only clock that
> > > > > really needs to use CLK_OF_DECLARE() is any clock that's needed for
> > > > > the scheduler timer. Other than that, everything else can be
> > > > > initialized by a normal driver. Including UART clocks. I can get into
> > > > > more specifics if you go down this path.
> > > > >
> > > > > So, that's how fw_devlink could help here if you massage
> > > > > drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
> > > > > have to set fw_devlink=on in the kernel commandline though (it's work
> > > > > in progress to set this by default). There are some additional details
> > > > > here about keeping clocks on, but we can discuss the solution for that
> > > > > if it becomes an issue.
> > > > >
> > > > > > I see exactly two
> > > > > > options to solve this:
> > > > > >
> > > > > > a) do not describe the link and keep RTC clock enabled somehow.
> > > > > > (my initial patchset)
> > > > > > b) describe the link, but ignore it during boot.
> > > > > > (what I'm trying to do here)
> > > > > >
> > > > >
> > > > > Even if you completely ignore fw_devlink, why not just model this
> > > > > clock as a fixed-clock in DT for this specific machine?
> > > > >
> > > > > It's clearly expecting the clock to be an always on fixed clock.
> > > >
> > > > Yes. SoC runs unreliably with this. Downstream vendor kernel does
> > > > not contain a clock driver for the squarewave pin of the RTC (i.e.
> > > > their driver does not yet contain 1373e77b4f10) and just works.
> > > > Upstream kernel disables the RTC's squarewave and then goes into
> > > > reboot loop because of watchdog going crazy.
> > > >
> > > > > This will also remove the need for adding "boot-clock-frequencies"
> > > > > binding. "fixed-clocks" devices are initialized very early on
> > > > > (they use CLK_OF_DECLARE too) even without their parents probing
> > > > > (not sure I agree with this, but this is how it works now).
> > > > >
> > > > > Something like:
> > > > >
> > > > > rtc: m41t62@68 {
> > > > > compatible = "st,m41t62";
> > > > > reg = <0x68>;
> > > > >
> > > > > clock-ckil {
> > > > > compatible = "fixed-clock";
> > > > > #clock-cells = <0>;
> > > > > clock-frequency = <32768>;
> > > > > };
> > > > > };
> > > > >
> > > > > I hope this helps.
> > > >
> > > > This looks like a complex way of my initial patchset with
> > > > 'protected-clocks' property replaced by a fixed-clock
> > > > node. RTC driver needs to check if that exists and avoid
> > > > registering its own clock.
> > >
> > > If anything, I'd argue this is a lot more simpler because it avoids
> > > adding a new DT binding, it avoids changes to drivers/clk/clk.c.
> >
> > My original patch [0] is a two liner, which does not change
> > drivers/clk/clk.c and protected-clocks is a standard property
> > from [1]. I think you confused this with the boot-clock-frequencies
> > approach :)
>
> I think my confusion was that you wanted to do both [0] and [1]
> because of them being threaded and not having v1/v2.
Yes, I send PATCHv1 and an incomplete RFCv2 labled RFC, sorry.
> Just to clarify, I'm not NAKing any patch here. I'm just explaining
> how things work and giving options because I was CCed. I'll leave it
> to Stephen/Rob to decide what they want to accept.
and I try to figure out how to get this thing supported upstream,
which blocks the whole series adding a new system on module and
five similar boards using it :)
> But I can see the point in Rob's request for wanting the DT to capture
> the real hardware connections correctly.
>
> > [0] https://lore.kernel.org/linux-devicetree/20210222171247.97609-2-sebastian.reichel@collabora.com/
> > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt.
> >
> > > Instead of checking for "protected-clocks" you just check for this
> > > child node (or just any child node). Also, technically if you set the
> > > CLK_IGNORE_UNUSED flag for the clock, you don't even need to do any
> > > explicit checking in the RTC driver as long as some other driver
> > > doesn't try to get this clock and turn it on/off.
> >
> > Child nodes are part of DT binding, so the information about the
> > potential clock subnode also needs to be added to the RTC binding.
> > It also changes the reference point from referencing the RTC node
> > to referencing a subnode, which seems a bit inconsistent to me.
>
> Sure, you can add a child node to the RTC binding.
/me is confused. This is what you suggested, see "Something like:"
> But it's not a new DT property binding (if you go with option 2).
well of course binding for RTC and for fixed-clock already exist,
but RTC binding needs to be changed to support a fixed-clock
sub-node (binding, not driver!). If Rob is fine with this I can
take that route.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [RFC] clk: add boot clock support
2021-04-05 22:43 ` Sebastian Reichel
@ 2021-04-05 23:51 ` Saravana Kannan
0 siblings, 0 replies; 28+ messages in thread
From: Saravana Kannan @ 2021-04-05 23:51 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Rob Herring, Michael Turquette, Stephen Boyd, linux-clk,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
Collabora Kernel ML
On Mon, Apr 5, 2021 at 3:43 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Tue, Mar 30, 2021 at 10:05:45AM -0700, Saravana Kannan wrote:
> > On Tue, Mar 30, 2021 at 2:09 AM Sebastian Reichel
> > <sebastian.reichel@collabora.com> wrote:
> > > On Mon, Mar 29, 2021 at 05:36:11PM -0700, Saravana Kannan wrote:
> > > > On Mon, Mar 29, 2021 at 2:53 PM Sebastian Reichel
> > > > <sebastian.reichel@collabora.com> wrote:
> > > > > On Mon, Mar 29, 2021 at 01:03:20PM -0700, Saravana Kannan wrote:
> > > > > > On Fri, Mar 26, 2021 at 2:52 AM Sebastian Reichel
> > > > > > <sebastian.reichel@collabora.com> wrote:
> > > > > > > On Thu, Mar 25, 2021 at 06:55:52PM -0700, Saravana Kannan wrote:
> > > > > > > > On Thu, Mar 25, 2021 at 6:27 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > > > > On Thu, Mar 18, 2021 at 10:03:18PM +0100, Sebastian Reichel wrote:
> > > > > > > > > > On Congatec's QMX6 system on module one of the i.MX6 fixed clocks
> > > > > > > > > > is provided by an I2C RTC. Specifying this properly results in a
> > > > > > > > > > circular dependency, since the I2C RTC (and thus its clock) cannot
> > > > > > > > > > be initialized without the i.MX6 clock controller being initialized.
> > > > > > > > > >
> > > > > > > > > > With current code the following path is executed when i.MX6 clock
> > > > > > > > > > controller is probed (and ckil clock is specified to be the I2C RTC
> > > > > > > > > > via DT):
> > > > > > > > > >
> > > > > > > > > > 1. imx6q_obtain_fixed_clk_hw(ccm_node, "ckil", 0);
> > > > > > > > > > 2. of_clk_get_by_name(ccm_node, "ckil");
> > > > > > > > > > 3. __of_clk_get(ccm_node, 0, ccm_node->full_name, "ckil");
> > > > > > > > > > 4. of_clk_get_hw(ccm_node, 0, "ckil")
> > > > > > > > > > 5. spec = of_parse_clkspec(ccm_node, 0, "ckil"); // get phandle
> > > > > > > > > > 6. of_clk_get_hw_from_clkspec(&spec); // returns -EPROBE_DEFER
> > > > > > > > > > 7. error is propagated back, i.MX6q clock controller is probe deferred
> > > > > > > > > > 8. I2C controller is never initialized without clock controller
> > > > > > > > > > I2C RTC is never initialized without I2C controller
> > > > > > > > > > CKIL clock is never initialized without I2C RTC
> > > > > > > > > > clock controller is never initialized without CKIL
> > > > > > > > > >
> > > > > > > > > > To fix the circular dependency this registers a dummy clock when
> > > > > > > > > > the RTC clock is tried to be acquired. The dummy clock will later
> > > > > > > > > > be unregistered when the proper clock is registered for the RTC
> > > > > > > > > > DT node. IIUIC clk_core_reparent_orphans() will take care of
> > > > > > > > > > fixing up the clock tree.
> > > > > > > > > >
> > > > > > > > > > NOTE: For now the patch is compile tested only. If this approach
> > > > > > > > > > is the correct one I will do some testing and properly submit this.
> > > > > > > > > > You can find all the details about the hardware in the following
> > > > > > > > > > patchset:
> > > > > > > > > >
> > > > > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-1-sebastian.reichel@collabora.com/
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > > > > > > > > ---
> > > > > > > > > > .../bindings/clock/clock-bindings.txt | 7 +
> > > > > > > > > > drivers/clk/clk.c | 146 ++++++++++++++++++
> > > > > > > > > > 2 files changed, 153 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > > > index f2ea53832ac6..66d67ff4aa0f 100644
> > > > > > > > > > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > > > > > > > @@ -32,6 +32,13 @@ clock-output-names: Recommended to be a list of strings of clock output signal
> > > > > > > > > > Clock consumer nodes must never directly reference
> > > > > > > > > > the provider's clock-output-names property.
> > > > > > > > > >
> > > > > > > > > > +boot-clock-frequencies: This property is used to specify that a clock is enabled
> > > > > > > > > > + by default with the provided frequency at boot time. This
> > > > > > > > > > + is required to break circular clock dependencies. For clock
> > > > > > > > > > + providers with #clock-cells = 0 this is a single u32
> > > > > > > > > > + with the frequency in Hz. Otherwise it's a list of
> > > > > > > > > > + clock cell specifier + frequency in Hz.
> > > > > > > > >
> > > > > > > > > Seems alright to me. I hadn't thought about the aspect of needing to
> > > > > > > > > know the frequency. Other cases probably don't as you only need the
> > > > > > > > > clocks once both components have registered.
> > > > > > > > >
> > > > > > > > > Note this could be lost being threaded in the other series.
> > > > > > > >
> > > > > > > > I read this thread and tried to understand it. But my head isn't right
> > > > > > > > today (lack of sleep) so I couldn't wrap my head around it. I'll look
> > > > > > > > at it again after the weekend. In the meantime, Sebastian can you
> > > > > > > > please point me to the DT file and the specific device nodes (names or
> > > > > > > > line number) where this cycle is present?
> > > > > > >
> > > > > > > I have not yet sent an updated DT file, but if you look at this
> > > > > > > submission:
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-devicetree/20210222171247.97609-7-sebastian.reichel@collabora.com/
> > > > > > >
> > > > > > > There is a node
> > > > > > >
> > > > > > > rtc: m41t62@68 { compatible = "st,m41t62"; };
> > > > > > >
> > > > > > > That is an I2C RTC, which provides a 32.768 kHz clock by default
> > > > > > > (i.e. after power loss). This clock signal is used to provide the
> > > > > > > i.MX6 CKIL:
> > > > > > >
> > > > > > > ------------------------------------
> > > > > > > &clks {
> > > > > > > clocks = <&rtc>;
> > > > > > > clock-names = "ckil";
> > > > > > > };
> > > > > > > ------------------------------------
> > > > > > >
> > > > > > > > Keeping a clock on until all its consumers probe is part of my TODO
> > > > > > > > list (next item after fw_devlink=on lands). I already have it working
> > > > > > > > in AOSP, but need to clean it up for upstream. fw_devlink can also
> > > > > > > > break *some* cycles (not all). So I'm wondering if the kernel will
> > > > > > > > solve this automatically soon(ish). If it can solve it automatically,
> > > > > > > > I'd rather not add new DT bindings because it'll make it more work for
> > > > > > > > fw_devlink.
> > > > > > >
> > > > > > > As written above on Congatec QMX6 an I2C RTC provides one of the
> > > > > > > SoC's input frequencies. The SoC basically expects that frequency
> > > > > > > to be always enabled and this is what it works like before clock
> > > > > > > support had been added to the RTC driver.
> > > > > >
> > > > > > Thanks. I skimmed through the RTC driver code and
> > > > > > imx6q_obtain_fixed_clk_hw() and the DT files.
> > > > > >
> > > > > > >
> > > > > > > With the link properly being described the Kernel tries to probe
> > > > > > > the SoC's clock controller during early boot. Then it tries to get a
> > > > > > > reference to the linked clock, using imx6q_obtain_fixed_clk_hw()
> > > > > > > and that returns -EPROBE_DEFER (because the RTC driver has not
> > > > > > > yet been probed).
> > > > > >
> > > > > > But the RTC (which is a proper I2C device) will never probe before
> > > > > > CLK_OF_DECLARE() initializes the core clock controller. So, it's not
> > > > > > clear how "protected-clocks" helps here since it doesn't change
> > > > > > whether you get -EPROBE_DEFER from imx6q_obtain_fixed_clk_hw() (which
> > > > > > is called from the CLK_OF_DECLARE() callback). Oof... I see what you
> > > > > > are doing with of_clk_register_boot_clk(). You are having the consumer
> > > > > > register its own clock and then use it. Kinda beats the whole point of
> > > > > > describing the link in the first place.
> > > > >
> > > > > I agree, that it does not make sense from a code point of view for
> > > > > this platform. All of this is just to make the DT look correct.
> > > > > From a platform point of view the most logical way is to handle the
> > > > > RTC clock as do-not-touch always enabled fixed-clock.
> > > > >
> > > > > > > Without the clock controller basically none of
> > > > > > > the i.MX6 SoC drivers can probe including the I2C driver. Without
> > > > > > > the I2C bus being registered, the RTC driver never probes and the
> > > > > > > boot process is stuck.
> > > > > > >
> > > > > > > I'm not sure how fw_devlink can help here.
> > > > > >
> > > > > > I'll explain how it'd help. Let's assume "fsl,imx6q-ccm" was
> > > > > > implemented as an actual platform device driver and not using
> > > > > > CLK_OF_DECLARE() to initialize ALL the clocks. I'll get to this
> > > > > > assumption later.
> > > > > >
> > > > > > In that case, fw_devlink will notice this cycle:
> > > > > > syntax: consumer -(reason)-> supplier
> > > > > > clks -(clocks property)-> rtc -(parent)-> i2c3 -(clocks property)-> clks.
> > > > > >
> > > > > > It'll then reason that it doesn't make sense for a device (clks) to
> > > > > > have a supplier (rtc) whose parent (i2c3) in turn depends on the
> > > > > > device (clks). It'll then drop the clks -> rtc dependency because
> > > > > > that's the most illogical one in terms of probing.
> > > > > >
> > > > > > So all you'd need to do is delete any -EPROBE defer you might do in
> > > > > > "fsl,imx6q-ccm" driver for "ckil". For cases where there's no cycle,
> > > > > > fw_devlink will make sure the supplier of ckil has probed first. For
> > > > > > cases where there's a cycle like this, it'll be smart enough to drop
> > > > > > this dependency during probe ordering.
> > > > >
> > > > > What do you mean drop? Anything using ckil will not be registered?
> > > > > That will basically kill the system within a few seconds, since the
> > > > > watchdog hardware uses ckil.
> > > >
> > > > No, it means that it won't block CCM on ckil. It's not a generic
> > > > "ignore dependency for all consumers of ckil". fw_devlink does this
> > > > specifically to the link that causes a probe dependency cycle.
> > >
> > > I still don't follow. If CCM proceeds booting without blocking on
> > > missing CCM,
> >
> > I think you meant to say missing CKIL,
>
> Yes.
>
> > > what would be the content of hws[IMX6QDL_CLK_CKIL]?
> > > What ensures, that the consumers get correct clock rates?
> >
> > I haven't dug into the IMX CCM driver, but my current understanding is
> > that the 32 KHz clock is the CKIL input coming into CCM and it's a
> > parent/ancestor to some/all of the CCM clocks.
>
> Right.
>
> > The clock framework allows you to register clocks before their
> > parents are registered (because clocks are messy and clock
> > providers can have cycles between them). So if the IMX CCM driver
> > is written correctly, it'd register the clock with the clock
> > framework saying "hey, my parent is clock CKIL from this other DT
> > node, connect us up when it's registered". I'll let you figure
> > out the details of the implementation.
>
> I've been told this is supposed to work in theory before, but nobody
> could point at an example. All drivers, that I checked end up with
> -EPROBE_DEFER on missing parent clocks, which is good enough for
> most dependency issues, but not for cyclic dependencies.
Ok I had some time so I looked up some examples for you. Look at
drivers/clk/qcom/dispcc-sm8250.c and see how it used .fw_name =
"bi_tcxo" for the parents. You'll also see that it never does a
clk_get() on "bi_tcxo". It does the same thing for a bunch of other
clock inputs too. See arch/arm64/boot/dts/qcom/sm8250.dtsi for the
node that lists the input clocks. Basically parent's fw_name should
match what you have in DT. Hope that helps.
> > Also, as I said before, the fixed-clock(s) will be available and
> > working before the RTC probes. So, either the CCM registers first or
> > the CKIL fixed-clock registers first and the same caller would
> > register the other. And then the clock framework will connect them up
> > and everything will continue working nicely.
>
> Yes, since fixed-clock is completley unrelated to RTC. Without
> further driver changes the RTC would still register a clock
> device and disable the clock because of it being unused.
Right, to avoid turning off the "unused" fixed clock, the RTC driver
will have to NOT register the same clock if you have a fixed-clock
child node.
>
> > > > > > I don't know enough about the clocks in imx6q to comment if you can
> > > > > > get away without using CLK_OF_DECLARE() at all. The only clock that
> > > > > > really needs to use CLK_OF_DECLARE() is any clock that's needed for
> > > > > > the scheduler timer. Other than that, everything else can be
> > > > > > initialized by a normal driver. Including UART clocks. I can get into
> > > > > > more specifics if you go down this path.
> > > > > >
> > > > > > So, that's how fw_devlink could help here if you massage
> > > > > > drivers/clk/imx/clk-imx6q.c to be a proper platform driver. You'll
> > > > > > have to set fw_devlink=on in the kernel commandline though (it's work
> > > > > > in progress to set this by default). There are some additional details
> > > > > > here about keeping clocks on, but we can discuss the solution for that
> > > > > > if it becomes an issue.
> > > > > >
> > > > > > > I see exactly two
> > > > > > > options to solve this:
> > > > > > >
> > > > > > > a) do not describe the link and keep RTC clock enabled somehow.
> > > > > > > (my initial patchset)
> > > > > > > b) describe the link, but ignore it during boot.
> > > > > > > (what I'm trying to do here)
> > > > > > >
> > > > > >
> > > > > > Even if you completely ignore fw_devlink, why not just model this
> > > > > > clock as a fixed-clock in DT for this specific machine?
> > > > > >
> > > > > > It's clearly expecting the clock to be an always on fixed clock.
> > > > >
> > > > > Yes. SoC runs unreliably with this. Downstream vendor kernel does
> > > > > not contain a clock driver for the squarewave pin of the RTC (i.e.
> > > > > their driver does not yet contain 1373e77b4f10) and just works.
> > > > > Upstream kernel disables the RTC's squarewave and then goes into
> > > > > reboot loop because of watchdog going crazy.
> > > > >
> > > > > > This will also remove the need for adding "boot-clock-frequencies"
> > > > > > binding. "fixed-clocks" devices are initialized very early on
> > > > > > (they use CLK_OF_DECLARE too) even without their parents probing
> > > > > > (not sure I agree with this, but this is how it works now).
> > > > > >
> > > > > > Something like:
> > > > > >
> > > > > > rtc: m41t62@68 {
> > > > > > compatible = "st,m41t62";
> > > > > > reg = <0x68>;
> > > > > >
> > > > > > clock-ckil {
> > > > > > compatible = "fixed-clock";
> > > > > > #clock-cells = <0>;
> > > > > > clock-frequency = <32768>;
> > > > > > };
> > > > > > };
> > > > > >
> > > > > > I hope this helps.
> > > > >
> > > > > This looks like a complex way of my initial patchset with
> > > > > 'protected-clocks' property replaced by a fixed-clock
> > > > > node. RTC driver needs to check if that exists and avoid
> > > > > registering its own clock.
> > > >
> > > > If anything, I'd argue this is a lot more simpler because it avoids
> > > > adding a new DT binding, it avoids changes to drivers/clk/clk.c.
> > >
> > > My original patch [0] is a two liner, which does not change
> > > drivers/clk/clk.c and protected-clocks is a standard property
> > > from [1]. I think you confused this with the boot-clock-frequencies
> > > approach :)
> >
> > I think my confusion was that you wanted to do both [0] and [1]
> > because of them being threaded and not having v1/v2.
>
> Yes, I send PATCHv1 and an incomplete RFCv2 labled RFC, sorry.
>
> > Just to clarify, I'm not NAKing any patch here. I'm just explaining
> > how things work and giving options because I was CCed. I'll leave it
> > to Stephen/Rob to decide what they want to accept.
>
> and I try to figure out how to get this thing supported upstream,
> which blocks the whole series adding a new system on module and
> five similar boards using it :)
>
> > But I can see the point in Rob's request for wanting the DT to capture
> > the real hardware connections correctly.
> >
> > > [0] https://lore.kernel.org/linux-devicetree/20210222171247.97609-2-sebastian.reichel@collabora.com/
> > > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt.
> > >
> > > > Instead of checking for "protected-clocks" you just check for this
> > > > child node (or just any child node). Also, technically if you set the
> > > > CLK_IGNORE_UNUSED flag for the clock, you don't even need to do any
> > > > explicit checking in the RTC driver as long as some other driver
> > > > doesn't try to get this clock and turn it on/off.
> > >
> > > Child nodes are part of DT binding, so the information about the
> > > potential clock subnode also needs to be added to the RTC binding.
> > > It also changes the reference point from referencing the RTC node
> > > to referencing a subnode, which seems a bit inconsistent to me.
> >
> > Sure, you can add a child node to the RTC binding.
>
> /me is confused. This is what you suggested, see "Something like:"
The "sure, you can do it" is referring to you saying you'll need to
update the RTC's binding doc to list the child node.
> > But it's not a new DT property binding (if you go with option 2).
>
> well of course binding for RTC and for fixed-clock already exist,
> but RTC binding needs to be changed to support a fixed-clock
> sub-node (binding, not driver!). If Rob is fine with this I can
> take that route.
-Saravana
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-04-05 23:51 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-22 17:12 [PATCHv1 0/6] Support for GE B1x5v2 Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 1/6] rtc: m41t80: add support for protected clock Sebastian Reichel
2021-02-22 21:20 ` Alexandre Belloni
2021-02-22 21:26 ` Alexandre Belloni
2021-02-23 1:26 ` Sebastian Reichel
2021-03-06 19:56 ` Rob Herring
2021-03-08 14:03 ` Sebastian Reichel
2021-03-16 21:51 ` Rob Herring
2021-03-18 21:03 ` [RFC] clk: add boot clock support Sebastian Reichel
2021-03-26 1:27 ` Rob Herring
2021-03-26 1:55 ` Saravana Kannan
2021-03-26 9:52 ` Sebastian Reichel
2021-03-29 20:03 ` Saravana Kannan
2021-03-29 21:53 ` Sebastian Reichel
2021-03-30 0:36 ` Saravana Kannan
2021-03-30 9:09 ` Sebastian Reichel
2021-03-30 17:05 ` Saravana Kannan
2021-04-05 22:43 ` Sebastian Reichel
2021-04-05 23:51 ` Saravana Kannan
2021-02-22 17:12 ` [PATCHv1 2/6] drm/imx: Add 8 pixel alignment fix Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 3/6] dt-bindings: vendor-prefixes: add congatec Sebastian Reichel
2021-03-06 19:57 ` Rob Herring
2021-02-22 17:12 ` [PATCHv1 4/6] dt-bindings: arm: fsl: add GE B1x5pv2 boards Sebastian Reichel
2021-03-06 19:58 ` Rob Herring
2021-02-22 17:12 ` [PATCHv1 5/6] dt-bindings: mtd: jedec,spi-nor: add sst25vf032b Sebastian Reichel
2021-02-23 0:15 ` Rob Herring
2021-02-23 1:33 ` Sebastian Reichel
2021-02-22 17:12 ` [PATCHv1 6/6] ARM: dts: imx6: Add GE B1x5v2 Sebastian Reichel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).