Re: [PATCH 2/2] drm/ingenic: Don't request full modeset if property is not modified

[Maxime Ripard]

Hi,

On Mon, Mar 29, 2021 at 06:50:46PM +0100, Paul Cercueil wrote:
> Avoid requesting a full modeset if the sharpness property is not
> modified, because then we don't actually need it.
> 
> Fixes: fc1acf317b01 ("drm/ingenic: Add support for the IPU")
> Cc:  # 5.8+
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/gpu/drm/ingenic/ingenic-ipu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c 
> b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> index 3b1091e7c0cd..95b665c4a7b0 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> @@ -640,10 +640,12 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane 
> *plane,
>  {
>   struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
>   struct drm_crtc_state *crtc_state;
> + bool mode_changed;
>  
>   if (property != ipu->sharpness_prop)
>   return -EINVAL;
>  
> + mode_changed = val != ipu->sharpness;
>   ipu->sharpness = val;
>  
>   if (state->crtc) {
> @@ -651,7 +653,7 @@ ingenic_ipu_plane_atomic_set_property(struct drm_plane 
> *plane,
>   if (WARN_ON(!crtc_state))
>   return -EINVAL;
>  
> - crtc_state->mode_changed = true;
> + crtc_state->mode_changed |= mode_changed;
>   }

I'd just change the condition from

if (state->crtc)

to

if (state->crtc && val != ipu->sharpness)

It's going to be easier to extend if you ever need to. Also, drivers
usually do this in atomic_check, is there a specific reason to do it in
atomic_set_property?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Maxime Ripard]

Hi,

On Fri, Apr 09, 2021 at 09:35:07PM +0800, Kevin Tang wrote:
> > > > > + }
> > > > > +
> > > > > + return MODE_OK;
> > > > > +}
> > > > > +
> > > > > +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > > +struct drm_atomic_state *state)
> > > > > +{
> > > > > + struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > > > +
> > > > > + sprd_dpu_init(dpu);
> > > > > +
> > > > > + sprd_dpi_init(dpu);
> > > > > +
> > > > > + enable_irq(dpu->ctx.irq);
> > > >
> > > > Shouldn't this be in enable_vblank? And I would assume that you would
> > > > have the interrupts enabled all the time, but disabled in your device?
> > > >
> > > It seems better to put in enable_vblank, i will try and test it... Thks
> > >
> > >   And I would assume that you would
> > > have the interrupts enabled all the time, but disabled in your device?
> > > [kevin]I don’t quite understand this, can you help me explain it in
> > > detail?
> >
> > You seem to have a register that enables and disables the interrupt in
> > that device. The way we usually deal with them in this case is just to
> > call request_irq in your bind/probe with the interrupts enabled at the
> > controller level, and mask them when needed at the device level by
> > clearing / setting that bit.
> >
>  Yeah, we have display controller interrupts setting and clear register.
> But the interrupts all been enabled in bootloader(eg, lk or uboot),
> if the interrupt handler is active in the probe/bind phase by request_irq,
> but the whole display pipeline is not ready, there maybe have some problems.

It's fairly common to clear / ack the interrupts from the device before
calling request_irq precisely to avoid that issue.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Maxime Ripard]

Hi,

On Thu, Apr 15, 2021 at 08:18:52AM +0800, Kevin Tang wrote:
> Maxime Ripard  于2021年3月24日周三 下午7:10写道：
> > > +static struct sprd_dpu *sprd_crtc_init(struct drm_device *drm,
> > > +  struct drm_plane *primary)
> > > +{
> > > + struct device_node *port;
> > > + struct sprd_dpu *dpu;
> > > +
> > > + /*
> > > +  * set crtc port so that drm_of_find_possible_crtcs call works
> > > +  */
> > > + port = of_parse_phandle(drm->dev->of_node, "ports", 0);
> > > + if (!port) {
> > > + drm_err(drm, "find 'ports' phandle of %s failed\n",
> > > +   drm->dev->of_node->full_name);
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > + of_node_put(port);
> >
> > The YAML binding should already make sure that your binding is sane, and
> > if you still get a DT that doesn't follow it, you have a whole lot of
> > other issues than whether ports is there :)
> >
> > > + dpu = drmm_crtc_alloc_with_planes(drm, struct sprd_dpu, base,
> > > + primary, NULL,
> > > + _crtc_funcs, NULL);
> > > + if (IS_ERR(dpu)) {
> > > + drm_err(drm, "failed to init crtc.\n");
> > > + return dpu;
> > > + }
> > > +
> > > + dpu->base.port = port;
> >
> > But you're still referencing it here, while you called of_node_put on it
> > already? You should only call it once you're done with it.
> 
>  of_node_put should be called after done with it, this maybe indeed be a bug.
> i will fix it.
> >
> >
> > I'm not really sure why you would need drm_of_find_possible_crtcs to
> > work then if you don't follow the OF-Graph bindings.
> 
> it scan all endports of encoder, if a matching crtc is found by
> OF-Graph bindings
> and then genarate the crtc mask, here is description:
> 41  /**
> 42   * drm_of_find_possible_crtcs - find the possible CRTCs for an encoder 
> port
> 43   * @dev: DRM device
> 44   * @port: encoder port to scan for endpoints
> 45   *
> 46   * Scan all endpoints attached to a port, locate their attached CRTCs,
> 47   * and generate the DRM mask of CRTCs which may be attached to this
> 48   * encoder.
> 49   *
> if we don't follow the OF-Graph bindings, crtc can't attched to encoder.

Yeah, what I'm actually confused about is why you would need the
of_parse_phandle call. You usually call drm_of_find_possible_crtcs with
the encoder device node, so from your MIPI-DSI driver in your case, and
with it's device->of_node pointer and it should work perfectly fine?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 5/6] dt-bindings: display: add Unisoc's mipi dsi controller bindings

[Maxime Ripard]

On Fri, Apr 09, 2021 at 08:23:19AM +0800, Kevin Tang wrote:
> Maxime Ripard  于2021年4月7日周三 下午6:46写道：
> 
> > On Wed, Mar 31, 2021 at 09:49:14AM +0800, Kevin Tang wrote:
> > > Hi Maxime,
> > >
> > > Maxime Ripard  于2021年3月24日周三 下午7:13写道：
> > >
> > > > On Mon, Feb 22, 2021 at 09:28:21PM +0800, Kevin Tang wrote:
> > > > > From: Kevin Tang 
> > > > >
> > > > > Adds MIPI DSI Controller
> > > > > support for Unisoc's display subsystem.
> > > > >
> > > > > Cc: Orson Zhai 
> > > > > Cc: Chunyan Zhang 
> > > > > Signed-off-by: Kevin Tang 
> > > > > Reviewed-by: Rob Herring 
> > > > > ---
> > > > >  .../display/sprd/sprd,sharkl3-dsi-host.yaml   | 102
> > ++
> > > > >  1 file changed, 102 insertions(+)
> > > > >  create mode 100644
> > > >
> > Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > > > >
> > > > > diff --git
> > > >
> > a/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > > >
> > b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > > > > new file mode 100644
> > > > > index 0..d439f688f
> > > > > --- /dev/null
> > > > > +++
> > > >
> > b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > > > > @@ -0,0 +1,102 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id:
> > > > http://devicetree.org/schemas/display/sprd/sprd,sharkl3-dsi-host.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Unisoc MIPI DSI Controller
> > > > > +
> > > > > +maintainers:
> > > > > +  - Kevin Tang 
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +const: sprd,sharkl3-dsi-host
> > > > > +
> > > > > +  reg:
> > > > > +maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +maxItems: 2
> > > > > +
> > > > > +  clocks:
> > > > > +minItems: 1
> > > > > +
> > > > > +  clock-names:
> > > > > +items:
> > > > > +  - const: clk_src_96m
> > > > > +
> > > > > +  power-domains:
> > > > > +maxItems: 1
> > > > > +
> > > > > +  ports:
> > > > > +type: object
> > > > > +
> > > > > +properties:
> > > > > +  "#address-cells":
> > > > > +const: 1
> > > > > +
> > > > > +  "#size-cells":
> > > > > +const: 0
> > > > > +
> > > > > +  port@0:
> > > > > +type: object
> > > > > +description:
> > > > > +  A port node with endpoint definitions as defined in
> > > > > +
> > Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > > > +  That port should be the input endpoint, usually coming
> > from
> > > > > +  the associated DPU.
> > > > > +  port@1:
> > > > > +type: object
> > > > > +description:
> > > > > +  A port node with endpoint definitions as defined in
> > > > > +
> > Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > > > +  That port should be the output endpoint, usually output to
> > > > > +  the associated panel.
> > > >
> > > > The DSI generic binding asks that peripherals that are controlled
> > > > through a DCS be a subnode of the MIPI-DSI bus, not through a port
> > > > endpoint.
> > > >
> > >  Our DSI controller don't support DCS now...
> >
> > I'm not sure I follow you, you mentionned in the patch 4 that you were
> > testing for a device to be in command mode, how would that work without
> > DCS support?
> >
> Sorry, I see DCS as DSC, pls ignore my previous comments.
> 
> dsi input node is display controller and dsi output node is panel,
> I still don't understand what it has to do with dcs? and it seems that
> other vendors also like this.
> 
> can you help provide some cases?

So the device tree is a tree organized through which bus controls which
device: Your DSI controller is accessed through a memory-mapped region
and is thus a child node of the main bus (I guess?) and then, since the
DSI panel is going to be controlled through the DSI controller and
MIPI-DCS, it needs to be a child of the display controller.

This is exactly what is being described here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/mipi-dsi-bus.txt#n42

The second port is thus not needed at all

Maxime


signature.asc
Description: PGP signature

[PATCH v2 1/5] drm/connector: Create a helper to attach the hdr_output_metadata property

[Maxime Ripard]

All the drivers that implement HDR output call pretty much the same
function to initialise the hdr_output_metadata property, and while the
creation of that property is in a helper, every driver uses the same
code to attach it.

Provide a helper for it as well

Reviewed-by: Harry Wentland 
Reviewed-by: Jernej Skrabec 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Rebased on latest drm-misc-next tag
  - Added the tags
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 +--
 drivers/gpu/drm/drm_connector.c   | 21 +++
 drivers/gpu/drm/i915/display/intel_hdmi.c |  3 +--
 include/drm/drm_connector.h   |  1 +
 5 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 55e39b462a5e..1e22ce718010 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7078,9 +7078,7 @@ void amdgpu_dm_connector_init_helper(struct 
amdgpu_display_manager *dm,
if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
connector_type == DRM_MODE_CONNECTOR_eDP) {
-   drm_object_attach_property(
-   >base.base,
-   dm->ddev->mode_config.hdr_output_metadata_property, 0);
+   
drm_connector_attach_hdr_output_metadata_property(>base);
 
if (!aconnector->mst_port)

drm_connector_attach_vrr_capable_property(>base);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index dda4fa9a1a08..f24bbb840dbf 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2492,8 +2492,7 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
drm_connector_attach_max_bpc_property(connector, 8, 16);
 
if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe)
-   drm_object_attach_property(>base,
-   
connector->dev->mode_config.hdr_output_metadata_property, 0);
+   drm_connector_attach_hdr_output_metadata_property(connector);
 
drm_connector_attach_encoder(connector, hdmi->bridge.encoder);
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 7631f76e7f34..a4aa2d87af35 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2150,6 +2150,27 @@ int drm_connector_attach_max_bpc_property(struct 
drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_hdr_output_metadata_property - attach 
"HDR_OUTPUT_METADA" property
+ * @connector: connector to attach the property on.
+ *
+ * This is used to allow the userspace to send HDR Metadata to the
+ * driver.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector)
+{
+   struct drm_device *dev = connector->dev;
+   struct drm_property *prop = 
dev->mode_config.hdr_output_metadata_property;
+
+   drm_object_attach_property(>base, prop, 0);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 95919d325b0b..f2f1b025e6ba 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2965,8 +2965,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, 
struct drm_connector *c
drm_connector_attach_content_type_property(connector);
 
if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-   drm_object_attach_property(>base,
-   
connector->dev->mode_config.hdr_output_metadata_property, 0);
+   drm_connector_attach_hdr_output_metadata_property(connector);
 
if (!HAS_GMCH(dev_priv))
drm_connector_attach_max_bpc_property(connector, 8, 12);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..32172dab8427 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1671,6 +1671,7 @@ int drm_connector_attach_scaling_mode_property(struct 
drm_connector *connector,
   u32 scaling_mode_mask);
 int drm_connector_attach_vrr_capable_property(
struct drm_connector *connector);
+int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*con

[PATCH v2 2/5] drm/connector: Add helper to compare HDR metadata

[Maxime Ripard]

All the drivers that support the HDR metadata property have a similar
function to compare the metadata from one connector state to the next,
and force a mode change if they differ.

All these functions run pretty much the same code, so let's turn it into
an helper that can be shared across those drivers.

Reviewed-by: Harry Wentland 
Reviewed-by: Jernej Skrabec 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Rebased on latest drm-misc-next tag
  - Added the tags
  - Fix build breakage on amdgpu
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +--
 drivers/gpu/drm/drm_connector.c   | 28 +++
 drivers/gpu/drm/i915/display/intel_atomic.c   | 13 +
 include/drm/drm_connector.h   |  2 ++
 5 files changed, 34 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1e22ce718010..ed1972fb531d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5967,25 +5967,6 @@ static int fill_hdr_info_packet(const struct 
drm_connector_state *state,
return 0;
 }
 
-static bool
-is_hdr_metadata_different(const struct drm_connector_state *old_state,
- const struct drm_connector_state *new_state)
-{
-   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
-   struct drm_property_blob *new_blob = new_state->hdr_output_metadata;
-
-   if (old_blob != new_blob) {
-   if (old_blob && new_blob &&
-   old_blob->length == new_blob->length)
-   return memcmp(old_blob->data, new_blob->data,
- old_blob->length);
-
-   return true;
-   }
-
-   return false;
-}
-
 static int
 amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
 struct drm_atomic_state *state)
@@ -6003,7 +5984,7 @@ amdgpu_dm_connector_atomic_check(struct drm_connector 
*conn,
if (!crtc)
return 0;
 
-   if (is_hdr_metadata_different(old_con_state, new_con_state)) {
+   if (!drm_connector_atomic_hdr_metadata_equal(old_con_state, 
new_con_state)) {
struct dc_info_packet hdr_infopacket;
 
ret = fill_hdr_info_packet(new_con_state, _infopacket);
@@ -8357,7 +8338,7 @@ static void amdgpu_dm_atomic_commit_tail(struct 
drm_atomic_state *state)
  dm_old_crtc_state->abm_level;
 
hdr_changed =
-   is_hdr_metadata_different(old_con_state, new_con_state);
+   !drm_connector_atomic_hdr_metadata_equal(old_con_state, 
new_con_state);
 
if (!scaling_changed && !abm_changed && !hdr_changed)
continue;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index f24bbb840dbf..f871e33c2fc9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2395,21 +2395,6 @@ static int dw_hdmi_connector_get_modes(struct 
drm_connector *connector)
return ret;
 }
 
-static bool hdr_metadata_equal(const struct drm_connector_state *old_state,
-  const struct drm_connector_state *new_state)
-{
-   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
-   struct drm_property_blob *new_blob = new_state->hdr_output_metadata;
-
-   if (!old_blob || !new_blob)
-   return old_blob == new_blob;
-
-   if (old_blob->length != new_blob->length)
-   return false;
-
-   return !memcmp(old_blob->data, new_blob->data, old_blob->length);
-}
-
 static int dw_hdmi_connector_atomic_check(struct drm_connector *connector,
  struct drm_atomic_state *state)
 {
@@ -2423,7 +2408,7 @@ static int dw_hdmi_connector_atomic_check(struct 
drm_connector *connector,
if (!crtc)
return 0;
 
-   if (!hdr_metadata_equal(old_state, new_state)) {
+   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
crtc_state = drm_atomic_get_crtc_state(state, crtc);
if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a4aa2d87af35..b13021b1b128 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2171,6 +2171,34 @@ int 
drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 }
 EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
 
+/**
+ * drm_connector_atomic_hdr_metadata_equal - checks if the hdr metadata changed

[PATCH v2 4/5] drm/connector: Add a helper to attach the colorspace property

[Maxime Ripard]

The intel driver uses the same logic to attach the Colorspace property
in multiple places and we'll need it in vc4 too. Let's move that common
code in a helper.

Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - New patch
---
 drivers/gpu/drm/drm_connector.c   | 20 +++
 .../gpu/drm/i915/display/intel_connector.c|  6 ++
 include/drm/drm_connector.h   |  1 +
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b13021b1b128..6a20b249e533 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2171,6 +2171,26 @@ int 
drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 }
 EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
 
+/**
+ * drm_connector_attach_colorspace_property - attach "Colorspace" property
+ * @connector: connector to attach the property on.
+ *
+ * This is used to allow the userspace to signal the output colorspace
+ * to the driver.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_colorspace_property(struct drm_connector *connector)
+{
+   struct drm_property *prop = connector->colorspace_property;
+
+   drm_object_attach_property(>base, prop, 
DRM_MODE_COLORIMETRY_DEFAULT);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_colorspace_property);
+
 /**
  * drm_connector_atomic_hdr_metadata_equal - checks if the hdr metadata changed
  * @old_state: old connector state to compare
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c 
b/drivers/gpu/drm/i915/display/intel_connector.c
index d5ceb7bdc14b..9bed1ccecea0 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -282,14 +282,12 @@ void
 intel_attach_hdmi_colorspace_property(struct drm_connector *connector)
 {
if (!drm_mode_create_hdmi_colorspace_property(connector))
-   drm_object_attach_property(>base,
-  connector->colorspace_property, 0);
+   drm_connector_attach_colorspace_property(connector);
 }
 
 void
 intel_attach_dp_colorspace_property(struct drm_connector *connector)
 {
if (!drm_mode_create_dp_colorspace_property(connector))
-   drm_object_attach_property(>base,
-  connector->colorspace_property, 0);
+   drm_connector_attach_colorspace_property(connector);
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1f51d73ca715..714d1a01c065 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1671,6 +1671,7 @@ int drm_connector_attach_scaling_mode_property(struct 
drm_connector *connector,
   u32 scaling_mode_mask);
 int drm_connector_attach_vrr_capable_property(
struct drm_connector *connector);
+int drm_connector_attach_colorspace_property(struct drm_connector *connector);
 int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector);
 bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state 
*old_state,
 struct drm_connector_state 
*new_state);
-- 
2.30.2

[PATCH v2 3/5] drm/vc4: Add HDR metadata property to the VC5 HDMI connectors

[Maxime Ripard]

From: Dave Stevenson 

Now that we can export deeper colour depths, add in the signalling
for HDR metadata.

Signed-off-by: Dave Stevenson 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Rebased on latest drm-misc-next tag
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 53 ++
 drivers/gpu/drm/vc4/vc4_hdmi.h |  3 ++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 1fda574579af..a33fa1662588 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -214,6 +214,31 @@ static int vc4_hdmi_connector_get_modes(struct 
drm_connector *connector)
return ret;
 }
 
+static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+   struct drm_connector_state *old_state =
+   drm_atomic_get_old_connector_state(state, connector);
+   struct drm_connector_state *new_state =
+   drm_atomic_get_new_connector_state(state, connector);
+   struct drm_crtc *crtc = new_state->crtc;
+
+   if (!crtc)
+   return 0;
+
+   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
+   struct drm_crtc_state *crtc_state;
+
+   crtc_state = drm_atomic_get_crtc_state(state, crtc);
+   if (IS_ERR(crtc_state))
+   return PTR_ERR(crtc_state);
+
+   crtc_state->mode_changed = true;
+   }
+
+   return 0;
+}
+
 static void vc4_hdmi_connector_reset(struct drm_connector *connector)
 {
struct vc4_hdmi_connector_state *old_state =
@@ -263,6 +288,7 @@ static const struct drm_connector_funcs 
vc4_hdmi_connector_funcs = {
 
 static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs 
= {
.get_modes = vc4_hdmi_connector_get_modes,
+   .atomic_check = vc4_hdmi_connector_atomic_check,
 };
 
 static int vc4_hdmi_connector_init(struct drm_device *dev,
@@ -299,6 +325,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
connector->interlace_allowed = 1;
connector->doublescan_allowed = 0;
 
+   if (vc4_hdmi->variant->supports_hdr)
+   drm_connector_attach_hdr_output_metadata_property(connector);
+
drm_connector_attach_encoder(connector, encoder);
 
return 0;
@@ -432,6 +461,25 @@ static void vc4_hdmi_set_audio_infoframe(struct 
drm_encoder *encoder)
vc4_hdmi_write_infoframe(encoder, );
 }
 
+static void vc4_hdmi_set_hdr_infoframe(struct drm_encoder *encoder)
+{
+   struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+   struct drm_connector *connector = _hdmi->connector;
+   struct drm_connector_state *conn_state = connector->state;
+   union hdmi_infoframe frame;
+
+   if (!vc4_hdmi->variant->supports_hdr)
+   return;
+
+   if (!conn_state->hdr_output_metadata)
+   return;
+
+   if (drm_hdmi_infoframe_set_hdr_metadata(, conn_state))
+   return;
+
+   vc4_hdmi_write_infoframe(encoder, );
+}
+
 static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 {
struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
@@ -444,6 +492,8 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder 
*encoder)
 */
if (vc4_hdmi->audio.streaming)
vc4_hdmi_set_audio_infoframe(encoder);
+
+   vc4_hdmi_set_hdr_infoframe(encoder);
 }
 
 static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
@@ -2102,6 +2152,7 @@ static const struct vc4_hdmi_variant bcm2835_variant = {
.phy_rng_enable = vc4_hdmi_phy_rng_enable,
.phy_rng_disable= vc4_hdmi_phy_rng_disable,
.channel_map= vc4_hdmi_channel_map,
+   .supports_hdr   = false,
 };
 
 static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = {
@@ -2129,6 +2180,7 @@ static const struct vc4_hdmi_variant 
bcm2711_hdmi0_variant = {
.phy_rng_enable = vc5_hdmi_phy_rng_enable,
.phy_rng_disable= vc5_hdmi_phy_rng_disable,
.channel_map= vc5_hdmi_channel_map,
+   .supports_hdr   = true,
 };
 
 static const struct vc4_hdmi_variant bcm2711_hdmi1_variant = {
@@ -2156,6 +2208,7 @@ static const struct vc4_hdmi_variant 
bcm2711_hdmi1_variant = {
.phy_rng_enable = vc5_hdmi_phy_rng_enable,
.phy_rng_disable= vc5_hdmi_phy_rng_disable,
.channel_map= vc5_hdmi_channel_map,
+   .supports_hdr   = true,
 };
 
 static const struct of_device_id vc4_hdmi_dt_match[] = {
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 3cebd1fd00fc..060bcaefbeb5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -99,6 +99,9 @@ struct vc4_hdmi_variant {
 
/* Callback to get chan

[PATCH v2 5/5] drm/vc4: hdmi: Signal the proper colorimetry info in the infoframe

[Maxime Ripard]

Our driver while supporting HDR didn't send the proper colorimetry info
in the AVI infoframe.

Let's add the property needed so that the userspace can let us know what
the colorspace is supposed to be.

Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - New patch
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index a33fa1662588..a22e17788074 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -226,7 +226,8 @@ static int vc4_hdmi_connector_atomic_check(struct 
drm_connector *connector,
if (!crtc)
return 0;
 
-   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
+   if (old_state->colorspace != new_state->colorspace ||
+   !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
struct drm_crtc_state *crtc_state;
 
crtc_state = drm_atomic_get_crtc_state(state, crtc);
@@ -316,6 +317,11 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
if (ret)
return ret;
 
+   ret = drm_mode_create_hdmi_colorspace_property(connector);
+   if (ret)
+   return ret;
+
+   drm_connector_attach_colorspace_property(connector);
drm_connector_attach_tv_margin_properties(connector);
drm_connector_attach_max_bpc_property(connector, 8, 12);
 
@@ -424,7 +430,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder 
*encoder)
   vc4_encoder->limited_rgb_range ?
   HDMI_QUANTIZATION_RANGE_LIMITED :
   HDMI_QUANTIZATION_RANGE_FULL);
-
+   drm_hdmi_avi_infoframe_colorspace(, cstate);
drm_hdmi_avi_infoframe_bars(, cstate);
 
vc4_hdmi_write_infoframe(encoder, );
-- 
2.30.2

[PATCH 1/2] clk: Introduce a clock request API

[Maxime Ripard]

It's not unusual to find clocks being shared across multiple devices
that need to change the rate depending on what the device is doing at a
given time.

The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
between its two HDMI controllers that share a clock that needs to be
raised depending on the output resolution of each controller.

The current clk_set_rate API doesn't really allow to support that case
since there's really no synchronisation between multiple users, it's
essentially a fire-and-forget solution.

clk_set_min_rate does allow for such a synchronisation, but has another
drawback: it doesn't allow to reduce the clock rate once the work is
over.

In our previous example, this means that if we were to raise the
resolution of one HDMI controller to the largest resolution and then
changing for a smaller one, we would still have the clock running at the
largest resolution rate resulting in a poor power-efficiency.

In order to address both issues, let's create an API that allows user to
create temporary requests to increase the rate to a minimum, before
going back to the initial rate once the request is done.

This introduces mainly two side-effects:

  * There's an interaction between clk_set_rate and requests. This has
been addressed by having clk_set_rate increasing the rate if it's
greater than what the requests asked for, and in any case changing
the rate the clock will return to once all the requests are done.

  * Similarly, clk_round_rate has been adjusted to take the requests
into account and return a rate that will be greater or equal to the
requested rates.

Signed-off-by: Maxime Ripard 
---
 drivers/clk/clk.c   | 121 
 include/linux/clk.h |   4 ++
 2 files changed, 125 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5052541a0986..4fd91a57e6db 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -77,12 +77,14 @@ struct clk_core {
unsigned intprotect_count;
unsigned long   min_rate;
unsigned long   max_rate;
+   unsigned long   default_request_rate;
unsigned long   accuracy;
int phase;
struct clk_duty duty;
struct hlist_head   children;
struct hlist_node   child_node;
struct hlist_head   clks;
+   struct list_headpending_requests;
unsigned intnotifier_count;
 #ifdef CONFIG_DEBUG_FS
struct dentry   *dentry;
@@ -105,6 +107,12 @@ struct clk {
struct hlist_node clks_node;
 };
 
+struct clk_request {
+   struct list_head list;
+   struct clk *clk;
+   unsigned long rate;
+};
+
 /***   runtime pm  ***/
 static int clk_pm_runtime_get(struct clk_core *core)
 {
@@ -1434,10 +1442,14 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, 
unsigned long rate)
 {
int ret;
struct clk_rate_request req;
+   struct clk_request *clk_req;
 
clk_core_get_boundaries(hw->core, _rate, _rate);
req.rate = rate;
 
+   list_for_each_entry(clk_req, >core->pending_requests, list)
+   req.min_rate = max(clk_req->rate, req.min_rate);
+
ret = clk_core_round_rate_nolock(hw->core, );
if (ret)
return 0;
@@ -1458,6 +1470,7 @@ EXPORT_SYMBOL_GPL(clk_hw_round_rate);
 long clk_round_rate(struct clk *clk, unsigned long rate)
 {
struct clk_rate_request req;
+   struct clk_request *clk_req;
int ret;
 
if (!clk)
@@ -1471,6 +1484,9 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
clk_core_get_boundaries(clk->core, _rate, _rate);
req.rate = rate;
 
+   list_for_each_entry(clk_req, >core->pending_requests, list)
+   req.min_rate = max(clk_req->rate, req.min_rate);
+
ret = clk_core_round_rate_nolock(clk->core, );
 
if (clk->exclusive_count)
@@ -1938,6 +1954,7 @@ static struct clk_core *clk_calc_new_rates(struct 
clk_core *core,
unsigned long new_rate;
unsigned long min_rate;
unsigned long max_rate;
+   struct clk_request *req;
int p_index = 0;
long ret;
 
@@ -1952,6 +1969,9 @@ static struct clk_core *clk_calc_new_rates(struct 
clk_core *core,
 
clk_core_get_boundaries(core, _rate, _rate);
 
+   list_for_each_entry(req, >pending_requests, list)
+   min_rate = max(req->rate, min_rate);
+
/* find the closest rate and parent clk/rate */
if (clk_core_can_round(core)) {
struct clk_rate_request req;
@@ -2156,6 +2176,7 @@ static unsigned long 
clk_core_req_round_rate_nolock(struct clk_core *core,
 {
int ret, cnt;
struct clk_rate_request req;
+   struct clk_request *clk_req;
 
lockdep_assert_held(_lock);
 
@@ -2170,6 +2191,9 @@ static unsigned long 
clk_core_req_

[PATCH 2/2] drm/vc4: hdmi: Convert to the new clock request API

[Maxime Ripard]

The new clock request API allows us to increase the rate of the HSM
clock to match our pixel rate requirements while decreasing it when
we're done, resulting in a better power-efficiency.

Signed-off-by: Maxime Ripard 
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 19 ---
 drivers/gpu/drm/vc4/vc4_hdmi.h |  3 +++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 1fda574579af..244053de6150 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -473,7 +473,9 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct 
drm_encoder *encoder,
   HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
 
clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
+   clk_request_done(vc4_hdmi->bvb_req);
clk_disable_unprepare(vc4_hdmi->hsm_clock);
+   clk_request_done(vc4_hdmi->hsm_req);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
 
ret = pm_runtime_put(_hdmi->pdev->dev);
@@ -778,9 +780,9 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct 
drm_encoder *encoder,
 * pixel clock, but HSM ends up being the limiting factor.
 */
hsm_rate = max_t(unsigned long, 12000, (pixel_rate / 100) * 101);
-   ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
-   if (ret) {
-   DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
+   vc4_hdmi->hsm_req = clk_request_start(vc4_hdmi->hsm_clock, hsm_rate);
+   if (IS_ERR(vc4_hdmi->hsm_req)) {
+   DRM_ERROR("Failed to set HSM clock rate: %ld\n", 
PTR_ERR(vc4_hdmi->hsm_req));
return;
}
 
@@ -797,10 +799,11 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct 
drm_encoder *encoder,
 * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup
 * at 300MHz.
 */
-   ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock,
-  (hsm_rate > VC4_HSM_MID_CLOCK ? 15000 : 
7500));
-   if (ret) {
-   DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
+   vc4_hdmi->bvb_req = clk_request_start(vc4_hdmi->pixel_bvb_clock,
+ (hsm_rate > VC4_HSM_MID_CLOCK ? 
15000 : 7500));
+   if (IS_ERR(vc4_hdmi->bvb_req)) {
+   DRM_ERROR("Failed to set pixel bvb clock rate: %ld\n", 
PTR_ERR(vc4_hdmi->bvb_req));
+   clk_request_done(vc4_hdmi->hsm_req);
clk_disable_unprepare(vc4_hdmi->hsm_clock);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
return;
@@ -809,6 +812,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct 
drm_encoder *encoder,
ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
if (ret) {
DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
+   clk_request_done(vc4_hdmi->bvb_req);
+   clk_request_done(vc4_hdmi->hsm_req);
clk_disable_unprepare(vc4_hdmi->hsm_clock);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
return;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 3cebd1fd00fc..9ac4a2c751df 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -167,6 +167,9 @@ struct vc4_hdmi {
 
struct reset_control *reset;
 
+   struct clk_request *bvb_req;
+   struct clk_request *hsm_req;
+
struct debugfs_regset32 hdmi_regset;
struct debugfs_regset32 hd_regset;
 };
-- 
2.30.2

[PATCH 0/2] clk: Implement a clock request API

[Maxime Ripard]

Hi,

This is a follow-up of the discussion here:
https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/

This implements a mechanism to raise and lower clock rates based on consumer
workloads, with an example of such an implementation for the RaspberryPi4 HDMI
controller.

There's a couple of things worth discussing:

  - The name is in conflict with clk_request_rate, and even though it feels
like the right name to me, we should probably avoid any confusion

  - The code so far implements a policy of always going for the lowest rate
possible. While we don't have an use-case for something else, this should
maybe be made more flexible?

Let me know what you think
Maxime

Maxime Ripard (2):
  clk: Introduce a clock request API
  drm/vc4: hdmi: Convert to the new clock request API

 drivers/clk/clk.c  | 121 +
 drivers/gpu/drm/vc4/vc4_hdmi.c |  19 --
 drivers/gpu/drm/vc4/vc4_hdmi.h |   3 +
 include/linux/clk.h|   4 ++
 4 files changed, 140 insertions(+), 7 deletions(-)

-- 
2.30.2

Re: linux-next: Signed-off-by missing for commit in the arm-soc tree

[Maxime Ripard]

On Tue, Apr 06, 2021 at 11:44:27PM +0200, Arnd Bergmann wrote:
> On Tue, Apr 6, 2021 at 12:11 AM Stephen Rothwell  
> wrote:
> >
> > Hi all,
> >
> > On Tue, 6 Apr 2021 08:11:00 +1000 Stephen Rothwell  
> > wrote:
> > >
> > > Hi all,
> > >
> > > Commit
> > >
> > >   3b493ac0ac04 ("arm64: dts: allwinner: h6: Switch to macros for RSB 
> > > clock/reset indices")
> > >
> > > is missing a Signed-off-by from its committer.
> >
> > Sorry, that commit is in the arm-soc-fixes tree.
> 
> Thanks for the report, I've temporarily removed the sunx-fixes branch merge
> from the arm/fixes branch and will send the pull request without it.
> 
> Maxime, can you fix it up and resend the pull request?
> Feel free to add any other fixes that have come up since then.

I just did. Is there a way to prevent this from happening when one
rebase a branch that was partially committed by someone else?

git rebase --signoff doesn't seem to detect if there's already a SoB, so
it might produce duplicates which isn't great either.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 6/6] drm/sprd: add Unisoc's drm mipi dsi driver

[Maxime Ripard]

On Wed, Mar 31, 2021 at 09:47:12AM +0800, Kevin Tang wrote:
> > > diff --git a/drivers/gpu/drm/sprd/Makefile
> > b/drivers/gpu/drm/sprd/Makefile
> > > index 6c25bfa99..d49f4977b 100644
> > > --- a/drivers/gpu/drm/sprd/Makefile
> > > +++ b/drivers/gpu/drm/sprd/Makefile
> > > @@ -1,5 +1,8 @@
> > >  # SPDX-License-Identifier: GPL-2.0
> > >
> > >  obj-y := sprd_drm.o \
> > > - sprd_dpu.o
> > > -
> > > + sprd_dpu.o \
> > > + sprd_dsi.o \
> > > + dw_dsi_ctrl.o \
> > > + dw_dsi_ctrl_ppi.o \
> >
> > So it's a designware IP? There's a driver for it already that seems
> > fairly similar:
> > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >
> Our dw dsi controller is not a standard synopsys ip, we have updated a lot
> on the basic ip version,
> the entire control register is different, i have cc to drm/bridge reviewers
> and maintainers.

You should make it more obvious then in a comment or in the name of the
driver. If it's fairly different from the original IP from Synopsys,
maybe you should just drop the reference to the name?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 5/6] dt-bindings: display: add Unisoc's mipi dsi controller bindings

[Maxime Ripard]

On Wed, Mar 31, 2021 at 09:49:14AM +0800, Kevin Tang wrote:
> Hi Maxime,
> 
> Maxime Ripard  于2021年3月24日周三 下午7:13写道：
> 
> > On Mon, Feb 22, 2021 at 09:28:21PM +0800, Kevin Tang wrote:
> > > From: Kevin Tang 
> > >
> > > Adds MIPI DSI Controller
> > > support for Unisoc's display subsystem.
> > >
> > > Cc: Orson Zhai 
> > > Cc: Chunyan Zhang 
> > > Signed-off-by: Kevin Tang 
> > > Reviewed-by: Rob Herring 
> > > ---
> > >  .../display/sprd/sprd,sharkl3-dsi-host.yaml   | 102 ++
> > >  1 file changed, 102 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > >
> > > diff --git
> > a/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > > new file mode 100644
> > > index 0..d439f688f
> > > --- /dev/null
> > > +++
> > b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> > > @@ -0,0 +1,102 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id:
> > http://devicetree.org/schemas/display/sprd/sprd,sharkl3-dsi-host.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Unisoc MIPI DSI Controller
> > > +
> > > +maintainers:
> > > +  - Kevin Tang 
> > > +
> > > +properties:
> > > +  compatible:
> > > +const: sprd,sharkl3-dsi-host
> > > +
> > > +  reg:
> > > +maxItems: 1
> > > +
> > > +  interrupts:
> > > +maxItems: 2
> > > +
> > > +  clocks:
> > > +minItems: 1
> > > +
> > > +  clock-names:
> > > +items:
> > > +  - const: clk_src_96m
> > > +
> > > +  power-domains:
> > > +maxItems: 1
> > > +
> > > +  ports:
> > > +type: object
> > > +
> > > +properties:
> > > +  "#address-cells":
> > > +const: 1
> > > +
> > > +  "#size-cells":
> > > +const: 0
> > > +
> > > +  port@0:
> > > +type: object
> > > +description:
> > > +  A port node with endpoint definitions as defined in
> > > +  Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +  That port should be the input endpoint, usually coming from
> > > +  the associated DPU.
> > > +  port@1:
> > > +type: object
> > > +description:
> > > +  A port node with endpoint definitions as defined in
> > > +  Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +  That port should be the output endpoint, usually output to
> > > +  the associated panel.
> >
> > The DSI generic binding asks that peripherals that are controlled
> > through a DCS be a subnode of the MIPI-DSI bus, not through a port
> > endpoint.
> >
>  Our DSI controller don't support DCS now...

I'm not sure I follow you, you mentionned in the patch 4 that you were
testing for a device to be in command mode, how would that work without
DCS support?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Maxime Ripard]

Hi,

Adding Jörg, Will and Robin,

On Wed, Mar 31, 2021 at 09:21:19AM +0800, Kevin Tang wrote:
> > > +static u32 check_mmu_isr(struct sprd_dpu *dpu, u32 reg_val)
> > > +{
> > > + struct dpu_context *ctx = >ctx;
> > > + u32 mmu_mask = BIT_DPU_INT_MMU_VAOR_RD |
> > > + BIT_DPU_INT_MMU_VAOR_WR |
> > > + BIT_DPU_INT_MMU_INV_RD |
> > > + BIT_DPU_INT_MMU_INV_WR;
> > > + u32 val = reg_val & mmu_mask;
> > > + int i;
> > > +
> > > + if (val) {
> > > + drm_err(dpu->drm, "--- iommu interrupt err: 0x%04x ---\n",
> > val);
> > > +
> > > + if (val & BIT_DPU_INT_MMU_INV_RD)
> > > + drm_err(dpu->drm, "iommu invalid read error, addr:
> > 0x%08x\n",
> > > + readl(ctx->base + REG_MMU_INV_ADDR_RD));
> > > + if (val & BIT_DPU_INT_MMU_INV_WR)
> > > + drm_err(dpu->drm, "iommu invalid write error,
> > addr: 0x%08x\n",
> > > + readl(ctx->base + REG_MMU_INV_ADDR_WR));
> > > + if (val & BIT_DPU_INT_MMU_VAOR_RD)
> > > + drm_err(dpu->drm, "iommu va out of range read
> > error, addr: 0x%08x\n",
> > > + readl(ctx->base + REG_MMU_VAOR_ADDR_RD));
> > > + if (val & BIT_DPU_INT_MMU_VAOR_WR)
> > > + drm_err(dpu->drm, "iommu va out of range write
> > error, addr: 0x%08x\n",
> > > + readl(ctx->base + REG_MMU_VAOR_ADDR_WR));
> >
> > Is that the IOMMU page fault interrupt? I would expect it to be in the
> > iommu driver.
> 
> Our iommu driver is indeed an separate driver, and also in upstreaming,
> but iommu fault interrupts reporting by display controller, not iommu
>  itself,
> if use iommu_set_fault_handler() to hook in our reporting function, there
> must be cross-module access to h/w registers.

Can you explain a bit more the design of the hardware then? Each device
connected to the IOMMU has a status register (and an interrupt) that
reports when there's a fault?

I'd like to get an ack at least from the IOMMU maintainers and
reviewers.

> > > +static void sprd_dpi_init(struct sprd_dpu *dpu)
> > > +{
> > > + struct dpu_context *ctx = >ctx;
> > > + u32 int_mask = 0;
> > > + u32 reg_val;
> > > +
> > > + if (ctx->if_type == SPRD_DPU_IF_DPI) {
> > > + /* use dpi as interface */
> > > + dpu_reg_clr(ctx, REG_DPU_CFG0, BIT_DPU_IF_EDPI);
> > > + /* disable Halt function for SPRD DSI */
> > > + dpu_reg_clr(ctx, REG_DPI_CTRL, BIT_DPU_DPI_HALT_EN);
> > > + /* select te from external pad */
> > > + dpu_reg_set(ctx, REG_DPI_CTRL,
> > BIT_DPU_EDPI_FROM_EXTERNAL_PAD);
> > > +
> > > + /* set dpi timing */
> > > + reg_val = ctx->vm.hsync_len << 0 |
> > > +   ctx->vm.hback_porch << 8 |
> > > +   ctx->vm.hfront_porch << 20;
> > > + writel(reg_val, ctx->base + REG_DPI_H_TIMING);
> > > +
> > > + reg_val = ctx->vm.vsync_len << 0 |
> > > +   ctx->vm.vback_porch << 8 |
> > > +   ctx->vm.vfront_porch << 20;
> > > + writel(reg_val, ctx->base + REG_DPI_V_TIMING);
> > > +
> > > + if (ctx->vm.vsync_len + ctx->vm.vback_porch < 32)
> > > + drm_warn(dpu->drm, "Warning: (vsync + vbp) < 32, "
> > > + "underflow risk!\n");
> >
> > I don't think a warning is appropriate here. Maybe we should just
> > outright reject any mode that uses it?
> >
>  This issue has been fixed on the new soc, maybe I should remove it.

If it still requires a workaround on older SoC, you can definitely add
it but we should prevent any situation where the underflow might occur
instead of reporting it once we're there.

> > > +static enum drm_mode_status sprd_crtc_mode_valid(struct drm_crtc *crtc,
> > > + const struct drm_display_mode
> > *mode)
> > > +{
> > > + struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > +
> > > + drm_dbg(dpu->drm, "%s() mode: "DRM_MODE_FMT"\n", __func__,
> > DRM_MODE_ARG(mode));
> > > +
> > > + if (mode->type & DRM_MODE_TYPE_PREFERRED) {
> > > + drm_display_mode_to_videomode(mode, >ctx.vm);
> >
> > You don't seem to use that anywhere else? And that's a bit fragile,
> > nothing really guarantees that it's the mode you're going to use, and
> > even then it can be adjusted.
> >
>  drm_mode convert to video_mode is been use in "sprd_dpu_init" and
> "sprd_dpi_init "
>  Preferred mode should be fixed mode, we generally don’t adjust it.

That's not really the assumption DRM is built upon though. The userspace
is even allowed to setup its own mode and try to configure it, and your
driver should take that into account.

I'd just drop that mode_valid hook, and retrieve the videomode if you
need it in atomic_enable or 

Re: [linux-sunxi] [PATCH] arm64: dts: allwinner: h6: beelink-gs1: Remove ext. 32 kHz osc reference

[Maxime Ripard]

On Wed, Mar 31, 2021 at 03:43:37PM +0200, Clément Péron wrote:
> Hi Jernej,
> 
> On Tue, 30 Mar 2021 at 20:42, Jernej Skrabec  wrote:
> >
> > Although every Beelink GS1 seems to have external 32768 Hz oscillator,
> > it works only on one from four tested. There are more reports of RTC
> > issues elsewhere, like Armbian forum.
> >
> > One Beelink GS1 owner read RTC osc status register on Android which
> > shipped with the box. Reported value indicated problems with external
> > oscillator.
> >
> > In order to fix RTC and related issues (HDMI-CEC and suspend/resume with
> > Crust) on all boards, switch to internal oscillator.
> >
> > Fixes: 32507b868119 ("arm64: dts: allwinner: h6: Move ext. oscillator to 
> > board DTs")
> > Signed-off-by: Jernej Skrabec 
> 
> Tested-by: Clément Péron 

Applied, thanks!
Maxime


signature.asc
Description: PGP signature

Re: [RFC PATCH] arm64: dts: allwinner: a64/h5: Add CPU idle states

[Maxime Ripard]

Hi Samuel,

On Tue, Mar 23, 2021 at 11:44:50PM -0500, Samuel Holland wrote:
> On 3/22/21 8:56 PM, Andre Przywara wrote:
> >> I'm sending this patch as an RFC because it raises questions about how
> >> we handle firmware versioning. How far back does (or should) our support
> >> for old TF-A and Crust versions go?
> >>
> >> cpuidle has a problem that without working firmware support, CPUs will
> >> enter idle states and be unable to wake up. As a result, the system will
> >> hang at some point during boot, usually before getting to userspace.
> >>
> >> For over a year[0], TF-A has exposed the PSCI CPU_SUSPEND function when
> >> a SCPI implementation is present[1]. Implementing CPU_SUSPEND is
> >> required for implementing SYSTEM_SUSPEND[2], even if CPU_SUSPEND is not
> >> itself used for anything. 
> >>
> >> However, there was no code to actually wake up a CPU once it called the
> >> CPU_SUSPEND function, because I could not find the register providing
> >> the necessary information. The fact that CPU_SUSPEND was broken affected
> >> nobody, because nothing ever called it -- there were no idle states in
> >> the DTS. In hindsight, what I should have done was always return failure
> >> from sunxi_validate_power_state(), but that ship has long sailed.
> >>
> >> I finally found the elusive register and implemented the wakeup code
> >> earlier this month[3]. So now, CPU_SUSPEND actually works, if all of
> >> your firmware is up to date, and cpuidle works if you add the states in
> >> your device tree.
> >>
> >> Unfortunately, there is currently nothing verifying that compatibility.
> >> So you can get into four possible scenarios:
> >>   1) No idle states in DTS, any firmware => Linux works, with baseline
> >>  power consumption.
> >>   2) Idle states added to DTS, no Crust/SCPI => Linux works, but every
> >>  attempt to enter an idle state is rejected because CPU_SUSPEND is
> >>  not hooked up. So power consumption increases by a sizable amount.
> >>   3) Idle states added to DTS, "old" Crust/SCPI (before [3]) => Linux
> >>  fails to boot, because CPUs never return from idle states.
> >>   4) Idle states added to DTS, "new" Crust/SCPI (after [3]) => Linux
> >>  works, with improved power consumption compared to the baseline.
> >>
> >> Obviously, we want to prevent scenario 3 if possible.
> > 
> > So I think the core of the problem is that the DT describes some
> > firmware feature, but we have the DT bundled with the kernel, not the
> > firmware.
> 
> I would say the core problem is that the firmware lies about supporting
> PSCI CPU_SUSPEND. Linux shouldn't be calling CPU_SUSPEND if the firmware
> declares it as unavailable, regardless of what is in the DTS.

I would say we have two core problems :)

> (Technically, per the PSCI standard, CPU_SUSPEND is a mandatory
> function, but a quick survey of the TF-A platforms shows it is far from
> universally implemented.)

U-boot also implements CPU_SUSPEND but will return -1 if you don't have
an implementation. I guess that at the moment we shouldn't be reporting
it as supported if we don't

But I also agree with Andre here, we shouldn't list cpuidles
capabilities in the DTS if we don't always have them either.

> > So is there any way we can detect an older crust version in U-Boot,
> > then remove any potential idle states from the DT?
> 
> Let's assume that we are using a functioning SoC (H3) or the secure fuse
> is blown (A64) and therefore U-Boot cannot access SRAM A2. I can think
> of three ways it can learn about crust:
> 
> a) PSCI_FEATURES (e.g. is CPU_SUSPEND supported)
> b) Metadata in the FIT image
> c) Custom SMCs
> 
> TF-A has some additional methods available:
> 
> d) The SCPI-reported firmware version
> e) The magic number at the beginning of the firmware binary
> 
> > Granted, this requires recent U-Boot as well, but at least we could try
> > to mitigate the worst case a bit?
> 
> If we're okay with modifying firmware to solve this problem, then I
> propose the following solution:
> 
> 1) Version bump crust or change its magic number.
> 2) Modify TF-A to only report CPU_SUSPEND as available if it detects the
>new crust version. This would involve conditionally setting
>sunxi_scpi_psci_ops.validate_power_state, and updating psci_setup.c
>to also check for .validate_power_state when setting psci_caps.
> 3) Modify the Linux PSCI client to respect PSCI_FEATURES when setting
>psci_ops.cpu_suspend. cpuidle-psci checks for this function before
>setting up idle states.
> 4) Finally, after some time, add the idle states to the DTS.
> 
> In fact, this solution solves both scenarios 2 and 3, because it also
> takes care of the native PM implementation, which doesn't implement
> CPU_SUSPEND at all.
> 
> Does that sound workable?

If we can add the DT node at boot in crust (or in TF-A), then we don't
really need PSCI_FEATURES, and it would magically work once the firmware
is updated. It looks like a solid plan otherwise

Re: [PATCH] drm: DON'T require each CRTC to have a unique primary plane

[Maxime Ripard]

On Mon, Mar 29, 2021 at 04:15:28PM +0100, Paul Cercueil wrote:
> Hi Maxime,
> 
> Le lun. 29 mars 2021 à 16:07, Maxime Ripard  a écrit :
> > On Sat, Mar 27, 2021 at 11:22:14AM +, Paul Cercueil wrote:
> > >  The ingenic-drm driver has two mutually exclusive primary planes
> > >  already; so the fact that a CRTC must have one and only one primary
> > >  plane is an invalid assumption.
> > 
> > I mean, no? It's been documented for a while that a CRTC should only
> > have a single primary, so I'd say that the invalid assumption was that
> > it was possible to have multiple primary planes for a CRTC.
> 
> Documented where?
> 
> I did read the doc of "enum drm_plane_type" in , and the
> DRM_PLANE_TYPE_PRIMARY describes my two planes, so I went with that.

At least since 4.9, this was in the documentation generated for DRM:
https://elixir.bootlin.com/linux/v4.9.263/source/drivers/gpu/drm/drm_plane.c#L43

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] drm: DON'T require each CRTC to have a unique primary plane

[Maxime Ripard]

On Sat, Mar 27, 2021 at 11:22:14AM +, Paul Cercueil wrote:
> The ingenic-drm driver has two mutually exclusive primary planes
> already; so the fact that a CRTC must have one and only one primary
> plane is an invalid assumption.

I mean, no? It's been documented for a while that a CRTC should only
have a single primary, so I'd say that the invalid assumption was that
it was possible to have multiple primary planes for a CRTC.

Since it looks like you have two mutually exclusive planes, just expose
one and be done with it?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 11/17] ASoC: sunxi: sun8i-codec: clarify expression

[Maxime Ripard]

On Fri, Mar 26, 2021 at 04:59:21PM -0500, Pierre-Louis Bossart wrote:
> cppcheck warning:
> 
> sound/soc/sunxi/sun8i-codec.c:488:28: style: Clarify calculation
> precedence for '%' and '?'. [clarifyCalculation]
>  return sample_rate % 4000 ? 22579200 : 24576000;
>^
> 
> Signed-off-by: Pierre-Louis Bossart 

Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 5/6] dt-bindings: display: add Unisoc's mipi dsi controller bindings

[Maxime Ripard]

On Mon, Feb 22, 2021 at 09:28:21PM +0800, Kevin Tang wrote:
> From: Kevin Tang 
> 
> Adds MIPI DSI Controller
> support for Unisoc's display subsystem.
> 
> Cc: Orson Zhai 
> Cc: Chunyan Zhang 
> Signed-off-by: Kevin Tang 
> Reviewed-by: Rob Herring 
> ---
>  .../display/sprd/sprd,sharkl3-dsi-host.yaml   | 102 ++
>  1 file changed, 102 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml 
> b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> new file mode 100644
> index 0..d439f688f
> --- /dev/null
> +++ 
> b/Documentation/devicetree/bindings/display/sprd/sprd,sharkl3-dsi-host.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sprd/sprd,sharkl3-dsi-host.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Unisoc MIPI DSI Controller
> +
> +maintainers:
> +  - Kevin Tang 
> +
> +properties:
> +  compatible:
> +const: sprd,sharkl3-dsi-host
> +
> +  reg:
> +maxItems: 1
> +
> +  interrupts:
> +maxItems: 2
> +
> +  clocks:
> +minItems: 1
> +
> +  clock-names:
> +items:
> +  - const: clk_src_96m
> +
> +  power-domains:
> +maxItems: 1
> +
> +  ports:
> +type: object
> +
> +properties:
> +  "#address-cells":
> +const: 1
> +
> +  "#size-cells":
> +const: 0
> +
> +  port@0:
> +type: object
> +description:
> +  A port node with endpoint definitions as defined in
> +  Documentation/devicetree/bindings/media/video-interfaces.txt.
> +  That port should be the input endpoint, usually coming from
> +  the associated DPU.
> +  port@1:
> +type: object
> +description:
> +  A port node with endpoint definitions as defined in
> +  Documentation/devicetree/bindings/media/video-interfaces.txt.
> +  That port should be the output endpoint, usually output to
> +  the associated panel.

The DSI generic binding asks that peripherals that are controlled
through a DCS be a subnode of the MIPI-DSI bus, not through a port
endpoint.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Maxime Ripard]

Hi,

On Mon, Feb 22, 2021 at 09:28:20PM +0800, Kevin Tang wrote:
> Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem.
> It's support multi planes, scaler, rotation, PQ(Picture Quality) and more.
> 
> Cc: Orson Zhai 
> Cc: Chunyan Zhang 
> Signed-off-by: Kevin Tang 
> 
> v2:
>   - Use drm_xxx to replace all DRM_XXX.
>   - Use kzalloc to replace devm_kzalloc for sprd_dpu structure init.
> 
> v3:
>   - Remove dpu_layer stuff layer and commit layers by aotmic_update
> 
> v4:
>   - Use drmm_helpers to allocate crtc and planes.
>   - Move rotation enum definitions to crtc layer reg bitfields.
>   - Move allocate crtc and planes to bind funtion.

There's a number of checkpatch warnings, make sure to fix them

> ---
>  drivers/gpu/drm/sprd/Kconfig|   1 +
>  drivers/gpu/drm/sprd/Makefile   |   4 +-
>  drivers/gpu/drm/sprd/sprd_dpu.c | 964 
>  drivers/gpu/drm/sprd/sprd_dpu.h | 109 
>  drivers/gpu/drm/sprd/sprd_drm.c |   1 +
>  drivers/gpu/drm/sprd/sprd_drm.h |   2 +
>  6 files changed, 1079 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.c
>  create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.h
> 
> diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig
> index 6e80cc9f3..9b4ef9aea 100644
> --- a/drivers/gpu/drm/sprd/Kconfig
> +++ b/drivers/gpu/drm/sprd/Kconfig
> @@ -3,6 +3,7 @@ config DRM_SPRD
>   depends on ARCH_SPRD || COMPILE_TEST
>   depends on DRM && OF
>   select DRM_KMS_HELPER
> + select VIDEOMODE_HELPERS
>   select DRM_GEM_CMA_HELPER
>   select DRM_KMS_CMA_HELPER
>   select DRM_MIPI_DSI
> diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile
> index 86d95d93a..6c25bfa99 100644
> --- a/drivers/gpu/drm/sprd/Makefile
> +++ b/drivers/gpu/drm/sprd/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -subdir-ccflags-y += -I$(srctree)/$(src)
> +obj-y := sprd_drm.o \
> + sprd_dpu.o
>  
> -obj-y := sprd_drm.o
> diff --git a/drivers/gpu/drm/sprd/sprd_dpu.c b/drivers/gpu/drm/sprd/sprd_dpu.c
> new file mode 100644
> index 0..75b7e40d9
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_dpu.c
> @@ -0,0 +1,964 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Unisoc Inc.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "sprd_drm.h"
> +#include "sprd_dpu.h"
> +
> +/* Global control registers */
> +#define REG_DPU_CTRL 0x04
> +#define REG_DPU_CFG0 0x08
> +#define REG_PANEL_SIZE   0x20
> +#define REG_BLEND_SIZE   0x24
> +#define REG_BG_COLOR 0x2C
> +
> +/* Layer0 control registers */
> +#define REG_LAY_BASE_ADDR0   0x30
> +#define REG_LAY_BASE_ADDR1   0x34
> +#define REG_LAY_BASE_ADDR2   0x38
> +#define REG_LAY_CTRL 0x40
> +#define REG_LAY_SIZE 0x44
> +#define REG_LAY_PITCH0x48
> +#define REG_LAY_POS  0x4C
> +#define REG_LAY_ALPHA0x50
> +#define REG_LAY_CROP_START   0x5C
> +
> +/* Interrupt control registers */
> +#define REG_DPU_INT_EN   0x1E0
> +#define REG_DPU_INT_CLR  0x1E4
> +#define REG_DPU_INT_STS  0x1E8
> +
> +/* DPI control registers */
> +#define REG_DPI_CTRL 0x1F0
> +#define REG_DPI_H_TIMING 0x1F4
> +#define REG_DPI_V_TIMING 0x1F8
> +
> +/* MMU control registers */
> +#define REG_MMU_EN   0x800
> +#define REG_MMU_VPN_RANGE0x80C
> +#define REG_MMU_VAOR_ADDR_RD 0x818
> +#define REG_MMU_VAOR_ADDR_WR 0x81C
> +#define REG_MMU_INV_ADDR_RD  0x820
> +#define REG_MMU_INV_ADDR_WR  0x824
> +#define REG_MMU_PPN1 0x83C
> +#define REG_MMU_RANGE1   0x840
> +#define REG_MMU_PPN2 0x844
> +#define REG_MMU_RANGE2   0x848
> +
> +/* Global control bits */
> +#define BIT_DPU_RUN  BIT(0)
> +#define BIT_DPU_STOP BIT(1)
> +#define BIT_DPU_REG_UPDATE   BIT(2)
> +#define BIT_DPU_IF_EDPI  BIT(0)
> +
> +/* Layer control bits */
> +#define BIT_DPU_LAY_EN   BIT(0)
> +#define BIT_DPU_LAY_LAYER_ALPHA  (0x01 << 2)
> +#define BIT_DPU_LAY_COMBO_ALPHA  (0x02 << 2)
> +#define BIT_DPU_LAY_FORMAT_YUV422_2PLANE (0x00 << 4)
> +#define BIT_DPU_LAY_FORMAT_YUV420_2PLANE (0x01 << 4)
> +#define BIT_DPU_LAY_FORMAT_YUV420_3PLANE (0x02 << 4)
> +#define BIT_DPU_LAY_FORMAT_ARGB  (0x03 << 4)
> +#define BIT_DPU_LAY_FORMAT_RGB565(0x04 << 4)
> +#define BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3 (0x00 << 8)
> +#define BIT_DPU_LAY_DATA_ENDIAN_B3B2B1B0 (0x01 << 8)
> +#define BIT_DPU_LAY_NO_SWITCH   

Re: [PATCH v4 2/6] drm/sprd: add Unisoc's drm kms master

[Maxime Ripard]

Hi

On Mon, Feb 22, 2021 at 09:28:18PM +0800, Kevin Tang wrote:
> Adds drm support for the Unisoc's display subsystem.
> 
> This is drm kms driver, this driver provides support for the
> application framework in Android, Yocto and more.
> 
> Application framework can access Unisoc's display internel

^ internal

> peripherals through libdrm or libkms, it's test ok by modetest
> (DRM/KMS test tool) and Android HWComposer.
> 
> Cc: Orson Zhai 
> Cc: Chunyan Zhang 
> Signed-off-by: Kevin Tang 
> 
> v4:
>   - Move the devm_drm_dev_alloc to master_ops->bind function.
>   - The managed drmm_mode_config_init() it is no longer necessary for drivers 
> to explicitly call drm_mode_config_cleanup, so delete it.
> ---
>  drivers/gpu/drm/Kconfig |   2 +
>  drivers/gpu/drm/Makefile|   1 +
>  drivers/gpu/drm/sprd/Kconfig|  12 ++
>  drivers/gpu/drm/sprd/Makefile   |   5 +
>  drivers/gpu/drm/sprd/sprd_drm.c | 217 
>  drivers/gpu/drm/sprd/sprd_drm.h |  16 +++
>  6 files changed, 253 insertions(+)
>  create mode 100644 drivers/gpu/drm/sprd/Kconfig
>  create mode 100644 drivers/gpu/drm/sprd/Makefile
>  create mode 100644 drivers/gpu/drm/sprd/sprd_drm.c
>  create mode 100644 drivers/gpu/drm/sprd/sprd_drm.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 8bf103de1..9d6ce2867 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -382,6 +382,8 @@ source "drivers/gpu/drm/tidss/Kconfig"
>  
>  source "drivers/gpu/drm/xlnx/Kconfig"
>  
> +source "drivers/gpu/drm/sprd/Kconfig"
> +
>  # Keep legacy drivers last
>  
>  menuconfig DRM_LEGACY
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 02c229392..42d211d9c 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -126,3 +126,4 @@ obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
>  obj-$(CONFIG_DRM_MCDE) += mcde/
>  obj-$(CONFIG_DRM_TIDSS) += tidss/
>  obj-y+= xlnx/
> +obj-$(CONFIG_DRM_SPRD) += sprd/
> diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig
> new file mode 100644
> index 0..6e80cc9f3
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Kconfig
> @@ -0,0 +1,12 @@
> +config DRM_SPRD
> + tristate "DRM Support for Unisoc SoCs Platform"
> + depends on ARCH_SPRD || COMPILE_TEST
> + depends on DRM && OF
> + select DRM_KMS_HELPER
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_MIPI_DSI

I guess this should rather be moved to your DSI introduction patch?

> + help
> +   Choose this option if you have a Unisoc chipset.
> +   If M is selected the module will be called sprd_drm.
> +
> diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile
> new file mode 100644
> index 0..86d95d93a
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +subdir-ccflags-y += -I$(srctree)/$(src)

Is it really needed? I'm not seeing any header that aren't in the
include path already.

> +obj-y := sprd_drm.o
> diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
> new file mode 100644
> index 0..a1d3ed655
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Unisoc Inc.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "sprd_drm.h"
> +
> +#define DRIVER_NAME  "sprd"
> +#define DRIVER_DESC  "Spreadtrum SoCs' DRM Driver"
> +#define DRIVER_DATE  "20200201"
> +#define DRIVER_MAJOR 1
> +#define DRIVER_MINOR 0
> +
> +static const struct drm_mode_config_helper_funcs sprd_drm_mode_config_helper 
> = {
> + .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};
> +
> +static const struct drm_mode_config_funcs sprd_drm_mode_config_funcs = {
> + .fb_create = drm_gem_fb_create,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static void sprd_drm_mode_config_init(struct drm_device *drm)
> +{
> + drm->mode_config.min_width = 0;
> + drm->mode_config.min_height = 0;
> + drm->mode_config.max_width = 8192;
> + drm->mode_config.max_height = 8192;
> + drm->mode_config.allow_fb_modifiers = true;
> +
> + drm->mode_config.funcs = _drm_mode_config_funcs;
> + drm->mode_config.helper_private = _drm_mode_config_helper;
> +}
> +
> +DEFINE_DRM_GEM_CMA_FOPS(sprd_drm_fops);
> +
> +static struct drm_driver sprd_drm_drv = {
> + .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> + .fops   = _drm_fops,
> +
> + /* GEM Operations */
> + DRM_GEM_CMA_DRIVER_OPS,
> +
> + .name   = DRIVER_NAME,
> 

Re: [PATCH v1] MAINTAINERS: Update Maintainers of DRM Bridge Drivers

[Maxime Ripard]

On Wed, Mar 24, 2021 at 11:20:19AM +0100, Robert Foss wrote:
> Add myself as co-maintainer of DRM Bridge Drivers. Repository
> commit access has already been granted.
> 
> https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/338
> 
> Cc: Neil Armstrong 
> Cc: Laurent Pinchart 
> Cc: Jonas Karlman 
> Cc: Andrzej Hajda 
> Cc: Jernej Škrabec 
> Cc: Daniel Vetter 
> Signed-off-by: Robert Foss 

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 1/4] drm: sun4i: dsi: Use drm_of_find_panel_or_bridge

[Maxime Ripard]

On Wed, Mar 24, 2021 at 11:55:35AM +0200, Laurent Pinchart wrote:
> Hi Jagan,
> 
> On Wed, Mar 24, 2021 at 03:19:10PM +0530, Jagan Teki wrote:
> > On Wed, Mar 24, 2021 at 3:09 PM Laurent Pinchart wrote:
> > > On Wed, Mar 24, 2021 at 02:44:57PM +0530, Jagan Teki wrote:
> > > > On Wed, Mar 24, 2021 at 8:18 AM Samuel Holland wrote:
> > > > > On 3/23/21 5:53 PM, Laurent Pinchart wrote:
> > > > > > On Mon, Mar 22, 2021 at 07:31:49PM +0530, Jagan Teki wrote:
> > > > > >> Replace of_drm_find_panel with drm_of_find_panel_or_bridge
> > > > > >> for finding panel, this indeed help to find the bridge if
> > > > > >> bridge support added.
> > > > > >>
> > > > > >> Added NULL in bridge argument, same will replace with bridge
> > > > > >> parameter once bridge supported.
> > > > > >>
> > > > > >> Signed-off-by: Jagan Teki 
> > > > > >
> > > > > > Looks good, there should be no functional change.
> > > > >
> > > > > Actually this breaks all existing users of this driver, see below.
> > > > >
> > > > > > Reviewed-by: Laurent Pinchart 
> > > > > >
> > > > > >> ---
> > > > > >> Changes for v4, v3:
> > > > > >> - none
> > > > > >>
> > > > > >>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ---
> > > > > >>  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > >>
> > > > > >> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c 
> > > > > >> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > > >> index 4f5efcace68e..2e9e7b2d4145 100644
> > > > > >> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > > >> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > > >> @@ -21,6 +21,7 @@
> > > > > >>
> > > > > >>  #include 
> > > > > >>  #include 
> > > > > >> +#include 
> > > > > >>  #include 
> > > > > >>  #include 
> > > > > >>  #include 
> > > > > >> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct 
> > > > > >> mipi_dsi_host *host,
> > > > > >>  struct mipi_dsi_device *device)
> > > > > >>  {
> > > > > >>  struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > > >> -struct drm_panel *panel = 
> > > > > >> of_drm_find_panel(device->dev.of_node);
> > > > >
> > > > > This is using the OF node of the DSI device, which is a direct child 
> > > > > of
> > > > > the DSI host's OF node. There is no OF graph involved.
> > > > >
> > > > > >> +struct drm_panel *panel;
> > > > > >> +int ret;
> > > > > >> +
> > > > > >> +ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0,
> > > > > >> +  , NULL);
> > > > >
> > > > > However, this function expects to find the panel using OF graph. This
> > > > > does not work with existing device trees (PinePhone, PineTab) which do
> > > > > not use OF graph to connect the panel. And it cannot work, because the
> > > > > DSI host's binding specifies a single port: the input port from the
> > > > > display engine.
> > > >
> > > > Thanks for noticing this. I did understand your point and yes, I did
> > > > mention the updated pipeline in previous versions and forgot to add it
> > > > to this series.
> > > >
> > > > Here is the updated pipeline to make it work:
> > > >
> > > > https://patchwork.kernel.org/project/dri-devel/patch/20190524104252.20236-1-ja...@amarulasolutions.com/
> > > >
> > > > Let me know your comments on this, so I will add a patch for the
> > > > above-affected DTS files.
> > >
> > > DT is an ABI, we need to ensure backward compatibility. Changes in
> > > kernel drivers can't break devices that have an old DT.
> > 
> > Thanks for your point.
> > 
> > So, we need to choose APIs that would compatible with the old DT and
> > new DT changes. Am I correct?
> 
> Yes, that's correct.

However, I see no particular reason to change the DT binding in this
case. The DSI devices are supposed to be described through a subnode of
their DSI controller, that's the generic binding and except for very odd
devices (and a bridge like this one is certainly not one), I see no
reason to deviate from that.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2] ARM: dts: sun8i: h3: beelink-x2: Add power button

[Maxime Ripard]

hi,

On Tue, Mar 23, 2021 at 09:43:41PM +0100, Jernej Skrabec wrote:
> Beelink X2 has power button. Add node for it.
> 
> Signed-off-by: Jernej Skrabec 

Applied, thanks

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] drm/rockchip: Remove unused variable

[Maxime Ripard]

On Sat, Mar 20, 2021 at 02:20:56AM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> On Fri, Mar 19, 2021 at 04:29:20PM +0100, Maxime Ripard wrote:
> > Commit 977697e20b3d ("drm/atomic: Pass the full state to planes atomic
> > disable and update") added the old_state variable instead of what used
> > to be a parameter, but it also removed the sole user of that variable in
> > the vop_plane_atomic_update function leading to an usused variable.
> > Remove it.
> > 
> > Fixes: 977697e20b3d ("drm/atomic: Pass the full state to planes atomic 
> > disable and update")
> > Reported-by: Stephen Rothwell 
> > Signed-off-by: Maxime Ripard 
> 
> Reviewed-by: Laurent Pinchart 

Applie,d thanks for your review

Maxime


signature.asc
Description: PGP signature

[PATCH] drm/rockchip: Remove unused variable

[Maxime Ripard]

Commit 977697e20b3d ("drm/atomic: Pass the full state to planes atomic
disable and update") added the old_state variable instead of what used
to be a parameter, but it also removed the sole user of that variable in
the vop_plane_atomic_update function leading to an usused variable.
Remove it.

Fixes: 977697e20b3d ("drm/atomic: Pass the full state to planes atomic disable 
and update")
Reported-by: Stephen Rothwell 
Signed-off-by: Maxime Ripard 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 81c70d7a0471..64469439ddf2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -879,8 +879,6 @@ static void vop_plane_atomic_disable(struct drm_plane 
*plane,
 static void vop_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
 {
-   struct drm_plane_state *old_state = 
drm_atomic_get_old_plane_state(state,
-  
plane);
struct drm_plane_state *new_state = 
drm_atomic_get_new_plane_state(state,
   
plane);
struct drm_crtc *crtc = new_state->crtc;
-- 
2.30.2

[PATCH 3/3] drm/vc4: Add HDR metadata property to the VC5 HDMI connectors

[Maxime Ripard]

From: Dave Stevenson 

Now that we can export deeper colour depths, add in the signalling
for HDR metadata.

Signed-off-by: Dave Stevenson 
Signed-off-by: Maxime Ripard 
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 53 ++
 drivers/gpu/drm/vc4/vc4_hdmi.h |  3 ++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index eee9751009c2..c8846cf9dd24 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -214,6 +214,31 @@ static int vc4_hdmi_connector_get_modes(struct 
drm_connector *connector)
return ret;
 }
 
+static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+   struct drm_connector_state *old_state =
+   drm_atomic_get_old_connector_state(state, connector);
+   struct drm_connector_state *new_state =
+   drm_atomic_get_new_connector_state(state, connector);
+   struct drm_crtc *crtc = new_state->crtc;
+
+   if (!crtc)
+   return 0;
+
+   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
+   struct drm_crtc_state *crtc_state;
+
+   crtc_state = drm_atomic_get_crtc_state(state, crtc);
+   if (IS_ERR(crtc_state))
+   return PTR_ERR(crtc_state);
+
+   crtc_state->mode_changed = true;
+   }
+
+   return 0;
+}
+
 static void vc4_hdmi_connector_reset(struct drm_connector *connector)
 {
struct vc4_hdmi_connector_state *old_state =
@@ -263,6 +288,7 @@ static const struct drm_connector_funcs 
vc4_hdmi_connector_funcs = {
 
 static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs 
= {
.get_modes = vc4_hdmi_connector_get_modes,
+   .atomic_check = vc4_hdmi_connector_atomic_check,
 };
 
 static int vc4_hdmi_connector_init(struct drm_device *dev,
@@ -299,6 +325,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
connector->interlace_allowed = 1;
connector->doublescan_allowed = 0;
 
+   if (vc4_hdmi->variant->supports_hdr)
+   drm_connector_attach_hdr_output_metadata_property(connector);
+
drm_connector_attach_encoder(connector, encoder);
 
return 0;
@@ -432,6 +461,25 @@ static void vc4_hdmi_set_audio_infoframe(struct 
drm_encoder *encoder)
vc4_hdmi_write_infoframe(encoder, );
 }
 
+static void vc4_hdmi_set_hdr_infoframe(struct drm_encoder *encoder)
+{
+   struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+   struct drm_connector *connector = _hdmi->connector;
+   struct drm_connector_state *conn_state = connector->state;
+   union hdmi_infoframe frame;
+
+   if (!vc4_hdmi->variant->supports_hdr)
+   return;
+
+   if (!conn_state->hdr_output_metadata)
+   return;
+
+   if (drm_hdmi_infoframe_set_hdr_metadata(, conn_state))
+   return;
+
+   vc4_hdmi_write_infoframe(encoder, );
+}
+
 static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 {
struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
@@ -444,6 +492,8 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder 
*encoder)
 */
if (vc4_hdmi->audio.streaming)
vc4_hdmi_set_audio_infoframe(encoder);
+
+   vc4_hdmi_set_hdr_infoframe(encoder);
 }
 
 static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
@@ -2101,6 +2151,7 @@ static const struct vc4_hdmi_variant bcm2835_variant = {
.phy_rng_enable = vc4_hdmi_phy_rng_enable,
.phy_rng_disable= vc4_hdmi_phy_rng_disable,
.channel_map= vc4_hdmi_channel_map,
+   .supports_hdr   = false,
 };
 
 static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = {
@@ -2128,6 +2179,7 @@ static const struct vc4_hdmi_variant 
bcm2711_hdmi0_variant = {
.phy_rng_enable = vc5_hdmi_phy_rng_enable,
.phy_rng_disable= vc5_hdmi_phy_rng_disable,
.channel_map= vc5_hdmi_channel_map,
+   .supports_hdr   = true,
 };
 
 static const struct vc4_hdmi_variant bcm2711_hdmi1_variant = {
@@ -2155,6 +2207,7 @@ static const struct vc4_hdmi_variant 
bcm2711_hdmi1_variant = {
.phy_rng_enable = vc5_hdmi_phy_rng_enable,
.phy_rng_disable= vc5_hdmi_phy_rng_disable,
.channel_map= vc5_hdmi_channel_map,
+   .supports_hdr   = true,
 };
 
 static const struct of_device_id vc4_hdmi_dt_match[] = {
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 3cebd1fd00fc..060bcaefbeb5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -99,6 +99,9 @@ struct vc4_hdmi_variant {
 
/* Callback to get channel map */
u32 (*channel_map)(struct 

[PATCH 1/3] drm/connector: Create a helper to attach the hdr_output_metadata property

[Maxime Ripard]

All the drivers that implement HDR output call pretty much the same
function to initialise the hdr_output_metadata property, and while the
creation of that property is in a helper, every driver uses the same
code to attach it.

Provide a helper for it as well

Signed-off-by: Maxime Ripard 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 +--
 drivers/gpu/drm/drm_connector.c   | 21 +++
 drivers/gpu/drm/i915/display/intel_hdmi.c |  3 +--
 include/drm/drm_connector.h   |  1 +
 5 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 22124f76d0b5..06908a3cee0f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7017,9 +7017,7 @@ void amdgpu_dm_connector_init_helper(struct 
amdgpu_display_manager *dm,
if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
connector_type == DRM_MODE_CONNECTOR_eDP) {
-   drm_object_attach_property(
-   >base.base,
-   dm->ddev->mode_config.hdr_output_metadata_property, 0);
+   
drm_connector_attach_hdr_output_metadata_property(>base);
 
if (!aconnector->mst_port)

drm_connector_attach_vrr_capable_property(>base);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index dda4fa9a1a08..f24bbb840dbf 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2492,8 +2492,7 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
drm_connector_attach_max_bpc_property(connector, 8, 16);
 
if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe)
-   drm_object_attach_property(>base,
-   
connector->dev->mode_config.hdr_output_metadata_property, 0);
+   drm_connector_attach_hdr_output_metadata_property(connector);
 
drm_connector_attach_encoder(connector, hdmi->bridge.encoder);
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 98b6ec45ef96..e25248e23e18 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2149,6 +2149,27 @@ int drm_connector_attach_max_bpc_property(struct 
drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_hdr_output_metadata_property - attach 
"HDR_OUTPUT_METADA" property
+ * @connector: connector to attach the property on.
+ *
+ * This is used to allow the userspace to send HDR Metadata to the
+ * driver.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector)
+{
+   struct drm_device *dev = connector->dev;
+   struct drm_property *prop = 
dev->mode_config.hdr_output_metadata_property;
+
+   drm_object_attach_property(>base, prop, 0);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index c5959590562b..52c051efb7b7 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2958,8 +2958,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, 
struct drm_connector *c
drm_connector_attach_content_type_property(connector);
 
if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-   drm_object_attach_property(>base,
-   
connector->dev->mode_config.hdr_output_metadata_property, 0);
+   drm_connector_attach_hdr_output_metadata_property(connector);
 
if (!HAS_GMCH(dev_priv))
drm_connector_attach_max_bpc_property(connector, 8, 12);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..32172dab8427 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1671,6 +1671,7 @@ int drm_connector_attach_scaling_mode_property(struct 
drm_connector *connector,
   u32 scaling_mode_mask);
 int drm_connector_attach_vrr_capable_property(
struct drm_connector *connector);
+int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector);
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 int drm_mode_create_hdmi_colorspace_property(struct drm_connector *connec

[PATCH 2/3] drm/connector: Add helper to compare HDR metadata

[Maxime Ripard]

All the drivers that support the HDR metadata property have a similar
function to compare the metadata from one connector state to the next,
and force a mode change if they differ.

All these functions run pretty much the same code, so let's turn it into
an helper that can be shared across those drivers.

Signed-off-by: Maxime Ripard 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 21 +-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +--
 drivers/gpu/drm/drm_connector.c   | 28 +++
 drivers/gpu/drm/i915/display/intel_atomic.c   | 13 +
 include/drm/drm_connector.h   |  2 ++
 5 files changed, 33 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 06908a3cee0f..4eb5201e566a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5924,25 +5924,6 @@ static int fill_hdr_info_packet(const struct 
drm_connector_state *state,
return 0;
 }
 
-static bool
-is_hdr_metadata_different(const struct drm_connector_state *old_state,
- const struct drm_connector_state *new_state)
-{
-   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
-   struct drm_property_blob *new_blob = new_state->hdr_output_metadata;
-
-   if (old_blob != new_blob) {
-   if (old_blob && new_blob &&
-   old_blob->length == new_blob->length)
-   return memcmp(old_blob->data, new_blob->data,
- old_blob->length);
-
-   return true;
-   }
-
-   return false;
-}
-
 static int
 amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
 struct drm_atomic_state *state)
@@ -5960,7 +5941,7 @@ amdgpu_dm_connector_atomic_check(struct drm_connector 
*conn,
if (!crtc)
return 0;
 
-   if (is_hdr_metadata_different(old_con_state, new_con_state)) {
+   if (!drm_connector_atomic_hdr_metadata_equal(old_con_state, 
new_con_state)) {
struct dc_info_packet hdr_infopacket;
 
ret = fill_hdr_info_packet(new_con_state, _infopacket);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index f24bbb840dbf..f871e33c2fc9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2395,21 +2395,6 @@ static int dw_hdmi_connector_get_modes(struct 
drm_connector *connector)
return ret;
 }
 
-static bool hdr_metadata_equal(const struct drm_connector_state *old_state,
-  const struct drm_connector_state *new_state)
-{
-   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
-   struct drm_property_blob *new_blob = new_state->hdr_output_metadata;
-
-   if (!old_blob || !new_blob)
-   return old_blob == new_blob;
-
-   if (old_blob->length != new_blob->length)
-   return false;
-
-   return !memcmp(old_blob->data, new_blob->data, old_blob->length);
-}
-
 static int dw_hdmi_connector_atomic_check(struct drm_connector *connector,
  struct drm_atomic_state *state)
 {
@@ -2423,7 +2408,7 @@ static int dw_hdmi_connector_atomic_check(struct 
drm_connector *connector,
if (!crtc)
return 0;
 
-   if (!hdr_metadata_equal(old_state, new_state)) {
+   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
crtc_state = drm_atomic_get_crtc_state(state, crtc);
if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index e25248e23e18..d781a3a1e9bf 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2170,6 +2170,34 @@ int 
drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 }
 EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
 
+/**
+ * drm_connector_atomic_hdr_metadata_equal - checks if the hdr metadata changed
+ * @old_state: old connector state to compare
+ * @new_state: new connector state to compare
+ *
+ * This is used by HDR-enabled drivers to test whether the HDR metadata
+ * have changed between two different connector state (and thus probably
+ * requires a full blown mode change).
+ *
+ * Returns:
+ * True if the metadata are equal, False otherwise
+ */
+bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state 
*old_state,
+struct drm_connector_state 
*new_state)
+{
+   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
+   struct drm_property_blob *new_blob = new_sta

Re: [PATCH] dt-bindings: Clean-up undocumented compatible strings

[Maxime Ripard]

Hi,

On Tue, Mar 16, 2021 at 01:49:18PM -0600, Rob Herring wrote:
> Adding checks for undocumented compatible strings reveals a bunch of
> warnings in the DT binding examples. Fix the cases which are typos, just
> a mismatch between the schema and the example, or aren't documented at all.
> In a couple of cases, fixing the compatible revealed some schema errors
> which are fixed.
> 
> There's a bunch of others remaining after this which have bindings, but
> those aren't converted to schema yet.
> 
> Cc: Stephen Boyd 
> Cc: Maxime Ripard 
> Cc: Thierry Reding 
> Cc: Sam Ravnborg 
> Cc: Vinod Koul 
> Cc: Alexandre Belloni 
> Cc: Jonathan Cameron 
> Cc: Pavel Machek 
> Cc: Kishon Vijay Abraham I 
> Cc: Sebastian Reichel 
> Cc: Mark Brown 
> Cc: Greg Kroah-Hartman 
> Cc: linux-...@vger.kernel.org
> Cc: dmaeng...@vger.kernel.org
> Cc: linux-...@lists.infradead.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-l...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: linux-ser...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Rob Herring 
> ---
>  .../clock/allwinner,sun4i-a10-pll1-clk.yaml   |  2 +-
>  .../bindings/clock/milbeaut-clock.yaml| 12 +
>  .../bindings/display/brcm,bcm2835-dsi0.yaml   |  6 -
>  .../bindings/display/panel/panel-dpi.yaml |  2 +-
>  .../devicetree/bindings/dma/qcom,gpi.yaml |  2 +-
>  .../devicetree/bindings/i3c/i3c.yaml  |  7 ++---
>  .../iio/adc/brcm,iproc-static-adc.yaml|  5 
>  .../iio/gyroscope/nxp,fxas21002c.yaml |  2 +-
>  .../bindings/iio/light/upisemi,us5182.yaml|  4 +--
>  .../interrupt-controller/loongson,htpic.yaml  |  2 +-
>  .../devicetree/bindings/leds/leds-lgm.yaml| 26 ---
>  .../bindings/phy/ti,phy-j721e-wiz.yaml|  2 +-
>  .../bindings/power/supply/cw2015_battery.yaml |  2 +-
>  .../bindings/power/supply/power-supply.yaml   | 22 
>  .../devicetree/bindings/serial/serial.yaml|  2 +-
>  .../bindings/spi/amlogic,meson-gx-spicc.yaml  |  4 +--
>  .../bindings/spi/spi-controller.yaml  | 21 ---
>  .../devicetree/bindings/spi/spi-mux.yaml      |  8 ++
>  .../devicetree/bindings/spi/st,stm32-spi.yaml |  6 -
>  19 files changed, 58 insertions(+), 79 deletions(-)

Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH] dt-bindings: Drop type references on common properties

[Maxime Ripard]

On Tue, Mar 16, 2021 at 01:48:58PM -0600, Rob Herring wrote:
> Users of common properties shouldn't have a type definition as the
> common schemas already have one. Drop all the unnecessary type
> references in the tree.
> 
> A meta-schema update to catch these is pending.
> 
> Cc: Nicolas Saenz Julienne 
> Cc: Maxime Ripard 
> Cc: Linus Walleij 
> Cc: Bartosz Golaszewski 
> Cc: Bjorn Andersson 
> Cc: Krzysztof Kozlowski 
> Cc: Marc Kleine-Budde 
> Cc: "David S. Miller" 
> Cc: Jakub Kicinski 
> Cc: Srinivas Kandagatla 
> Cc: Ohad Ben-Cohen 
> Cc: Mark Brown 
> Cc: Cheng-Yi Chiang 
> Cc: Benson Leung 
> Cc: Zhang Rui 
> Cc: Daniel Lezcano 
> Cc: Greg Kroah-Hartman 
> Cc: Stefan Wahren 
> Cc: Masahiro Yamada 
> Cc: Odelu Kukatla 
> Cc: Alex Elder 
> Cc: Suman Anna 
> Cc: Kuninori Morimoto 
> Cc: Dmitry Baryshkov 
> Cc: linux-g...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Cc: linux-remotep...@vger.kernel.org
> Cc: alsa-de...@alsa-project.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Rob Herring 

Acked-by: Maxime Ripard 

Thanks!
Maxiem


signature.asc
Description: PGP signature

Re: [PATCH v7 2/2] hwspinlock: add sun6i hardware spinlock support

[Maxime Ripard]

On Sun, Mar 14, 2021 at 10:31:13AM +0100, Wilken Gottwalt wrote:
> Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> most of the sun6i compatible SoCs.
> 
> This unit provides at least 32 spinlocks in hardware. The implementation
> supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> reading a register and released by writing a 0 to it. This driver
> supports all 4 spinlock setups, but for now only the first setup (32
> locks) seem to exist in available devices. This spinlock unit is shared
> between all ARM cores and the embedded companion core. All of them can
> take/release a lock with a single cycle operation. It can be used to
> sync access to devices shared by the ARM cores and the companion core.
> 
> There are two ways to check if a lock is taken. The first way is to read
> a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> is returned, the caller has to try again. Which means the lock is taken.
> The second way is to read a 32bit wide status register where every bit
> represents one of the 32 first locks. According to the datasheets this
> status register supports only the 32 first locks. This is the reason the
> first way (lock read/write) approach is used to be able to cover all 256
> locks in future devices. The driver also reports the amount of supported
> locks via debugfs.
> 
> Signed-off-by: Wilken Gottwalt 

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v7 1/2] dt-bindings: hwlock: add sun6i_hwspinlock

[Maxime Ripard]

On Sun, Mar 14, 2021 at 10:30:49AM +0100, Wilken Gottwalt wrote:
> Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> compatible series SoCs.
> 
> Signed-off-by: Wilken Gottwalt 

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Maxime Ripard]

Hi!

On Mon, Mar 08, 2021 at 10:47:03AM +0800, 班涛 wrote:
> Maxime Ripard  于2021年3月5日周五 上午12:07写道：
> 
> > Hi,
> >
> > On Tue, Mar 02, 2021 at 08:37:37PM +0800, Ban Tao wrote:
> > > From: Ban Tao 
> > >
> > > The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> > > IP compared to the older Allwinner SoCs.
> > >
> > > Signed-off-by: Ban Tao 
> >
> > Thanks for sending an update.
> >
> > Like I said in the previous version though, without proper SoC support
> > upstream, that driver would effectively be dead code
> >
> 
> I'm not sure if the latest Linux code supports R818, A133, R329, V536 or
> V833,

It doesn't at the moment...

> But the driver supports most of the sun50i family of SoCs and some of the
> sun8i SoCs.
> Maybe I shouldn't submit this code right now...

... but if that driver can drive controllers found on other SoCs that we
support already, then you're very welcome to work on those first :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] ARM: dts: sun8i: h3: beelink-x2: Add power button

[Maxime Ripard]

Hi,

On Thu, Mar 11, 2021 at 10:59:35PM +0100, Jernej Škrabec wrote:
> Hi!
> 
> Dne ponedeljek, 08. marec 2021 ob 14:05:06 CET je Maxime Ripard napisal(a):
> > Hi
> > 
> > On Sat, Mar 06, 2021 at 09:36:11PM +0100, Jernej Skrabec wrote:
> > > Beelink X2 has power button. Add node for it.
> > > 
> > > Signed-off-by: Jernej Skrabec 
> > > ---
> > >  arch/arm/boot/dts/sun8i-h3-beelink-x2.dts | 11 +++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts 
> > > b/arch/arm/boot/dts/
> sun8i-h3-beelink-x2.dts
> > > index 62b5280ec093..4a2cb072ecf6 100644
> > > --- a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> > > +++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> > > @@ -111,6 +111,17 @@ spdif_out: spdif-out {
> > >   #sound-dai-cells = <0>;
> > >   compatible = "linux,spdif-dit";
> > >   };
> > > +
> > > + r_gpio_keys {
> > 
> > Underscores are not valid for node names (and will trigger a dtc warning
> > when running with W=1).
> 
> Unless I'm doing something wrong, I didn't get any warning with "make dtbs 
> W=1". In fact many H3 boards have a node with this name and not a single 
> warning is produced with this command for underscores (there are other 
> warnings though).

It looks like I've been remembering it wrong, and it's actually W=2 that
reports it, but it's here nonetheless :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 2/5] dt-bindings: timer: Add compatibles for sun50i timers

[Maxime Ripard]

On Sun, Mar 14, 2021 at 11:32:47PM -0500, Samuel Holland wrote:
> The sun50i SoCs contain timer blocks which are useful as broadcast
> clockevent sources. They each have 2 interrupts, matching the A23
> variant, so add the new compatible strings with the A23 compatible
> as a fallback.
> 
> Signed-off-by: Samuel Holland 

Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH 1/5] dt-bindings: timer: Simplify conditional expressions

[Maxime Ripard]

On Sun, Mar 14, 2021 at 11:32:46PM -0500, Samuel Holland wrote:
> The sun4i timer IP block has a variable number of interrupts based on
> the compatible. Use enums to combine the two sections for the existing
> 3-interrupt variants, and to simplify adding new compatible strings.
> 
> Signed-off-by: Samuel Holland 

Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH RESEND 0/2] Common protected-clocks implementation

[Maxime Ripard]

Hi,

On Tue, Mar 09, 2021 at 09:03:14AM +0100, Rasmus Villemoes wrote:
> On 03/09/2020 06.00, Samuel Holland wrote:
> > Stephen, Maxime,
> > 
> > You previously asked me to implement the protected-clocks property in a
> > driver-independent way:
> > 
> > https://www.spinics.net/lists/arm-kernel/msg753832.html
> > 
> > I provided an implementation 6 months ago, which I am resending now:
> > 
> > https://patchwork.kernel.org/patch/11398629/
> > 
> > Do you have any comments on it?
> 
> I'm also interested [1] in getting something like this supported in a
> generic fashion - i.e., being able to mark a clock as
> protected/critical/whatnot by just adding an appropriate property in the
> clock provider's DT node, but without modifying the driver to opt-in to
> handling it.
> 
> Now, as to this implementation, the commit 48d7f160b1 which added the
> common protected-clocks binding says
> 
>   For example, on some Qualcomm firmwares reading or writing certain clk
>   registers causes the entire system to reboot,
> 
> so I'm not sure handling protected-clocks by translating it to
> CLK_CRITICAL and thus calling prepare/enable on it is the right thing to
> do - clks that behave like above are truly "hands off, kernel", so the
> current driver-specific implementation of simply not registering those
> clocks seems to be the right thing to do - or at least the clk framework
> would need to be taught to not actually call any methods on such
> protected clocks.

The idea to use CLK_CRITICAL was also there to allow any clock to be
marked as protected, and not just the root ones. Indeed, by just
ignoring that clock, the parent clock could end up in a situation where
it doesn't have any (registered) child and thus would be disabled,
disabling the ignored clock as well. Reparenting could cause the same
issue.

Calling clk_prepare_enable just allows the kernel to track that it (and
thus its parent) should never be disabled, ever.

> For my use case, either "hands off kernel" or "make sure this clock is
> enabled" would work since the bootloader anyway enables the clock.

The current protected clocks semantics have been that the clock
shouldn't be disabled by the kernel, but "hands off kernel" clocks imply
a slightly different one. I would expect that the bootloader in this
case wouldn't expect any parent or rate (or phase, or any other
parameter really) change, while it might be ok for some other use cases
(like the one Samuel was trying to address for example).

And even if we wanted the kernel to behave that way (making sure there's
no way to change the rate, parents, phase, etc.), the kernel would have
to have knowledge of it to also propagate that restriction to the whole
chain of parents.

Maxim


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] dts: r40: add second ethernet support

[Maxime Ripard]

Hi,

On Tue, Mar 09, 2021 at 04:21:16AM +0300, Evgeny Boger wrote:
> R40 (aka V40, A40i, T3) has two different Ethernet IP
> called EMAC and GMAC.
> EMAC only support 10/100 Mbit in MII mode,
> while GMAC support both 10/100 (MII) and 10/100/1000 (RGMII).
> 
> In contrast to A10/A20 where GMAC and EMAC share the same pins
> making EMAC somewhat pointless, on R40 EMAC can be routed to port H.
> Both EMAC (on port H) and GMAC (on port A)
>  can be then enabled at the same time, allowing for two ethernet ports.
> 
> Signed-off-by: Evgeny Boger 
> ---
>  arch/arm/boot/dts/sun8i-r40.dtsi | 59 
>  1 file changed, 59 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi 
> b/arch/arm/boot/dts/sun8i-r40.dtsi
> index d5ad3b9efd12..c31386e421b1 100644
> --- a/arch/arm/boot/dts/sun8i-r40.dtsi
> +++ b/arch/arm/boot/dts/sun8i-r40.dtsi
> @@ -217,6 +217,20 @@
>   #size-cells = <1>;
>   ranges;
>  
> + sram_a: sram@0 {
> + compatible = "mmio-sram";
> + reg = <0x 0xc000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x 0xc000>;
> +
> + emac_sram: sram-section@8000 {
> + compatible = 
> "allwinner,sun4i-a10-sram-a3-a4";
> + reg = <0x8000 0x4000>;
> + status = "okay";
> + };
> + };
> +
>   sram_c: sram@1d0 {
>   compatible = "mmio-sram";
>   reg = <0x01d0 0xd>;
> @@ -541,6 +555,33 @@
>   drive-strength = <40>;
>   };
>  
> + emac_pa_pins: emac-pa-pins {
> + pins = "PA0", "PA1", "PA2",
> +"PA3", "PA4", "PA5", "PA6",
> +"PA7", "PA8", "PA9", "PA10",
> +"PA11", "PA12", "PA13", "PA14",
> +"PA15", "PA16";
> + function = "emac";
> + };
> +
> + emac_ph_pins: emac-ph-pins {
> + pins = "PH8", "PH9", "PH10", "PH11",
> +"PH14", "PH15", "PH16", "PH17",
> +"PH18","PH19", "PH20", "PH21",
> +"PH22", "PH23", "PH24", "PH25",
> +"PH26", "PH27";
> + function = "emac";
> + };
> +
> + emac_pa_pins: emac-pa-pins {
> + pins = "PA0", "PA1", "PA2",
> +"PA3", "PA4", "PA5", "PA6",
> +"PA7", "PA8", "PA9", "PA10",
> +"PA11", "PA12", "PA13", "PA14",
> +"PA15", "PA16";
> + function = "emac";
> + };
> +

I guess you forgot to remove that node?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] net: allwinner: reset control support

[Maxime Ripard]

Hi,

On Tue, Mar 09, 2021 at 04:21:15AM +0300, Evgeny Boger wrote:
> R40 (aka V40/A40i/T3) and A10/A20 share the same EMAC IP.
> However, on R40 the EMAC is gated by default.
> 
> Signed-off-by: Evgeny Boger 
> ---
>  .../net/allwinner,sun4i-a10-emac.yaml | 11 +++-
>  drivers/net/ethernet/allwinner/sun4i-emac.c   | 65 +--
>  2 files changed, 70 insertions(+), 6 deletions(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/net/allwinner,sun4i-a10-emac.yaml 
> b/Documentation/devicetree/bindings/net/allwinner,sun4i-a10-emac.yaml
> index 8d8560a67abf..27f99372d153 100644
> --- a/Documentation/devicetree/bindings/net/allwinner,sun4i-a10-emac.yaml
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun4i-a10-emac.yaml
> @@ -15,7 +15,12 @@ maintainers:
>  
>  properties:
>compatible:
> -const: allwinner,sun4i-a10-emac
> +oneOf:
> +  - const: allwinner,sun4i-a10-emac
> +  - const: allwinner,sun4i-r40-emac
> +  - items:
> +  - const: allwinner,sun4i-r40-emac
> +  - const: allwinner,sun4i-a10-emac

There's no need to handle the fallback case, it should have either one
of the two, not both.

The good news is that it simplifies the binding here too, since you can
use an enum

The DT binding modifications are usually in a separate patch too

>  
>reg:
>  maxItems: 1
> @@ -30,6 +35,9 @@ properties:
>  description: Phandle to the device SRAM
>  $ref: /schemas/types.yaml#/definitions/phandle-array
>  
> +  resets:
> +maxItems: 1
> +

You should make resets required for the R40 compatible too through an if
clause.

It looks good otherwise, thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support

[Maxime Ripard]

On Sat, Mar 06, 2021 at 12:05:22PM +0100, Wilken Gottwalt wrote:
> On Tue, 2 Mar 2021 18:20:02 +0100
> Maxime Ripard  wrote:
> 
> > Hi,
> > 
> > On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote:
> > > On Mon, 1 Mar 2021 14:13:05 +0100
> > > Maxime Ripard  wrote:
> > > 
> > > > On Sat, Feb 27, 2021 at 02:03:54PM +0100, Wilken Gottwalt wrote:
> > > > > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found 
> > > > > in
> > > > > most of the sun6i compatible SoCs.
> > > > >
> > > > > This unit provides at least 32 spinlocks in hardware. The 
> > > > > implementation
> > > > > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> > > > > reading a register and released by writing a 0 to it. This driver
> > > > > supports all 4 spinlock setups, but for now only the first setup (32
> > > > > locks) seem to exist in available devices. This spinlock unit is 
> > > > > shared
> > > > > between all ARM cores and the embedded companion core. All of them can
> > > > > take/release a lock with a single cycle operation. It can be used to
> > > > > sync access to devices shared by the ARM cores and the companion core.
> > > > >
> > > > > There are two ways to check if a lock is taken. The first way is to 
> > > > > read
> > > > > a lock. If a 0 is returned, the lock was free and is taken now. If an 
> > > > > 1
> > > > > is returned, the caller has to try again. Which means the lock is 
> > > > > taken.
> > > > > The second way is to read a 32bit wide status register where every bit
> > > > > represents one of the 32 first locks. According to the datasheets this
> > > > > status register supports only the 32 first locks. This is the reason 
> > > > > the
> > > > > first way (lock read/write) approach is used to be able to cover all 
> > > > > 256
> > > > > locks in future devices. The driver also reports the amount of 
> > > > > supported
> > > > > locks via debugfs.
> > > > >
> > > > > Signed-off-by: Wilken Gottwalt 
> > > 
> > > Nope, I had to replace the devm_hwspin_lock_register function by the
> > > hwspin_lock_register function because like Bjorn pointed out that it can
> > > fail and needs to handled correctly. And having a devm_* function does not
> > > play well with the non-devm clock/reset functions and winding back if an
> > > error occurs. It also messes with the call order in the remove function. 
> > > So
> > > I went back to the classic way where I have full control over the call 
> > > order.
> > 
> > If you're talking about the clock and reset line reassertion, I don't
> > really see what the trouble is. Sure, it's not going to be in the exact
> > same order in remove, but it's still going to execute in the proper
> > order (ie, clock disable, then reset disable, then clock put and reset
> > put). And you can use devm_add_action if you want to handle things
> > automatically.
> 
> See, in v5 zje result of devm_hwspin_lock_register was returned directly. The
> remove callback or the bank_fail/clk_fail labels would not run, if the 
> registering
> fails. In v6 it is fixed.

Yeah, and it's indeed an issue...

> + platform_set_drvdata(pdev, priv);
> +
> + return devm_hwspin_lock_register(>dev, priv->bank, 
> _hwspinlock_ops,
> +  SPINLOCK_BASE_ID, priv->nlocks);
> +bank_fail:
> + clk_disable_unprepare(priv->ahb_clk);
> +clk_fail:
> + reset_control_assert(priv->reset);
> +
> + return err;
> +}
> 
> So, is v6 fine for you even if it uses a more classic approach?

... but completely getting rid of the devm_ calls isn't a solution, if
that's what you're calling the more classic approach.

if (devm_hwspin_lock_register(..))
   goto bank_fail

return 0;

works, and using devm_add_action like I suggested works as well to fix
the issue you mentioned above

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 1/1] clk: sunxi: Demote non-conformant kernel-doc headers

[Maxime Ripard]

On Wed, Mar 03, 2021 at 04:17:02PM -0800, Stephen Boyd wrote:
> Quoting Lee Jones (2021-03-03 06:24:30)
> > Headers must describe their parameters.
> > 
> > Fixes the following W=1 kernel build warning(s):
> > 
> >  drivers/clk/sunxi/clk-sun9i-core.c:27: warning: Function parameter or 
> > member 'req' not described in 'sun9i_a80_get_pll4_factors'
> >  drivers/clk/sunxi/clk-sun9i-core.c:100: warning: Function parameter or 
> > member 'req' not described in 'sun9i_a80_get_gt_factors'
> >  drivers/clk/sunxi/clk-sun9i-core.c:155: warning: Function parameter or 
> > member 'req' not described in 'sun9i_a80_get_ahb_factors'
> >  drivers/clk/sunxi/clk-sun9i-core.c:235: warning: Function parameter or 
> > member 'req' not described in 'sun9i_a80_get_apb1_factors'
> >  drivers/clk/sunxi/clk-usb.c:22: warning: cannot understand function 
> > prototype: 'struct usb_reset_data '
> >  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or 
> > member 'req' not described in 'sun6i_get_ar100_factors'
> > 
> > Cc: "Emilio López" 
> > Cc: Michael Turquette 
> > Cc: Stephen Boyd 
> > Cc: Maxime Ripard 
> > Cc: Chen-Yu Tsai 
> > Cc: Jernej Skrabec 
> > Cc: Boris BREZILLON 
> > Cc: linux-...@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Signed-off-by: Lee Jones 
> > ---
> 
> Acked-by: Stephen Boyd 

Applied

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 4/4] arm64: dts: allwinner: h6: Use RSB for AXP805 PMIC connection

[Maxime Ripard]

On Sun, Jan 03, 2021 at 04:00:07AM -0600, Samuel Holland wrote:
> On boards where the only peripheral connected to PL0/PL1 is an X-Powers
> PMIC, configure the connection to use the RSB bus rather than the I2C
> bus. Compared to the I2C controller that shares the pins, the RSB
> controller allows a higher bus frequency, and it is more CPU-efficient.
> 
> Signed-off-by: Samuel Holland 

Applied, thanks

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 2/2] dts: r40: add second ethernet support

[Maxime Ripard]

Hi,

On Sun, Mar 07, 2021 at 06:13:53AM +0300, Evgeny Boger wrote:
> R40 (aka V40, A40i, T3) has two different Ethernet IP
> called EMAC and GMAC.
> EMAC only support 10/100 Mbit in MII mode,
> while GMAC support both 10/100 (MII) and 10/100/1000 (RGMII).
> 
> In contrast to A10/A20 where GMAC and EMAC share the same pins
> making EMAC somewhat pointless, on R40 EMAC can be routed to port H.
> Both EMAC (on port H) and GMAC (on port A)
>  can be then enabled at the same time, allowing for two ethernet ports.
> 
> Signed-off-by: Evgeny Boger 
> ---
>  arch/arm/boot/dts/sun8i-r40.dtsi | 53 
>  1 file changed, 53 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi 
> b/arch/arm/boot/dts/sun8i-r40.dtsi
> index d5ad3b9efd12..c102c1510012 100644
> --- a/arch/arm/boot/dts/sun8i-r40.dtsi
> +++ b/arch/arm/boot/dts/sun8i-r40.dtsi
> @@ -217,6 +217,20 @@
>   #size-cells = <1>;
>   ranges;
>  
> + sram_a: sram@0 {
> + compatible = "mmio-sram";
> + reg = <0x 0xc000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x 0xc000>;
> +
> + emac_sram: sram-section@8000 {
> + compatible = 
> "allwinner,sun4i-a10-sram-a3-a4";
> + reg = <0x8000 0x4000>;
> + status = "okay";
> + };
> + };
> +
>   sram_c: sram@1d0 {
>   compatible = "mmio-sram";
>   reg = <0x01d0 0xd>;
> @@ -541,6 +555,24 @@
>   drive-strength = <40>;
>   };
>  
> + emac_ph_pins: emac-ph-pins {
> + pins = "PH8", "PH9", "PH10", "PH11",
> +"PH14", "PH15", "PH16", "PH17",
> +"PH18","PH19", "PH20", "PH21",
> +"PH22", "PH23", "PH24", "PH25",
> +"PH26", "PH27";
> + function = "emac";
> + };
> +
> + emac_pa_pins: emac-pa-pins {
> + pins = "PA0", "PA1", "PA2",
> +"PA3", "PA4", "PA5", "PA6",
> +"PA7", "PA8", "PA9", "PA10",
> +"PA11", "PA12", "PA13", "PA14",
> +"PA15", "PA16";
> + function = "emac";
> + };
> +

These nodes should be order alphabetically 

>   i2c0_pins: i2c0-pins {
>   pins = "PB0", "PB1";
>   function = "i2c0";
> @@ -885,6 +917,27 @@
>   };
>   };
>  
> + emac: ethernet@1c0b000 {
> + syscon = <>;

Why is the syscon needed? You weren't using it in the driver

> + compatible = "allwinner,sun4i-a10-emac";
> + reg = <0x01c0b000 0x1000>;
> + interrupts = ;
> + clocks = < CLK_BUS_EMAC>;
> + resets = < RST_BUS_EMAC>;
> + allwinner,sram = <_sram 1>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_ph_pins>;

If there's several options, we really can't enforce a default here, it
should be in the board DTS.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 1/2] net: allwinner: reset control support

[Maxime Ripard]

Hi,

On Sun, Mar 07, 2021 at 06:13:51AM +0300, Evgeny Boger wrote:
> R40 (aka V40/A40i/T3) and A10/A20 share the same EMAC IP.
> However, on R40 the EMAC is gated by default.
> 
> Signed-off-by: Evgeny Boger 

On which device was it tested?

> ---
>  drivers/net/ethernet/allwinner/sun4i-emac.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c 
> b/drivers/net/ethernet/allwinner/sun4i-emac.c
> index 5ed80d9a6b9f..c0ae06dd922c 100644
> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "sun4i-emac.h"
> @@ -85,6 +86,7 @@ struct emac_board_info {
>   unsigned intlink;
>   unsigned intspeed;
>   unsigned intduplex;
> + struct reset_control *reset;

You should align this with the rest of the other fields 

>  
>   phy_interface_t phy_interface;
>  };
> @@ -791,6 +793,7 @@ static int emac_probe(struct platform_device *pdev)
>   struct net_device *ndev;
>   int ret = 0;
>   const char *mac_addr;
> + struct reset_control *reset;
>  
>   ndev = alloc_etherdev(sizeof(struct emac_board_info));
>   if (!ndev) {
> @@ -852,6 +855,19 @@ static int emac_probe(struct platform_device *pdev)
>   goto out_release_sram;
>   }
>  
> + reset = devm_reset_control_get_optional_exclusive(>dev, NULL);
> + if (IS_ERR(reset)) {
> + dev_err(>dev, "unable to request reset\n");
> + ret = -ENODEV;
> + goto out_release_sram;
> + }

Judging from your commit log, it's not really optional for the R40. The
way we usually deal with this is to have a structure associated with a
new compatible and have a flag tell if that compatible requires a reset
line or not.

The dt binding should also be amended to allow the reset property

> + db->reset = reset;
> + ret = reset_control_deassert(db->reset);
> + if (ret) {
> + dev_err(>dev, "could not deassert EMAC reset\n");
> + goto out_release_sram;
> + }
> +

The programming guidelines in the datasheet ask that the reset line must
be deasserted before the clock in enabled.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] ARM: dts: sun8i: h3: beelink-x2: Add power button

[Maxime Ripard]

Hi

On Sat, Mar 06, 2021 at 09:36:11PM +0100, Jernej Skrabec wrote:
> Beelink X2 has power button. Add node for it.
> 
> Signed-off-by: Jernej Skrabec 
> ---
>  arch/arm/boot/dts/sun8i-h3-beelink-x2.dts | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts 
> b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> index 62b5280ec093..4a2cb072ecf6 100644
> --- a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> @@ -111,6 +111,17 @@ spdif_out: spdif-out {
>   #sound-dai-cells = <0>;
>   compatible = "linux,spdif-dit";
>   };
> +
> + r_gpio_keys {

Underscores are not valid for node names (and will trigger a dtc warning
when running with W=1).

> + compatible = "gpio-keys";
> +
> + power {
> + label = "power";

IIRC the node name is used as a fallback when the label isn't there?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 2/2] pwm: sun8i-v536: document device tree bindings

[Maxime Ripard]

Hi,

On Tue, Mar 02, 2021 at 08:40:23PM +0800, Ban Tao wrote:
> From: Ban Tao 
> 
> This adds binding documentation for sun8i-v536 SoC PWM driver.
> 
> Signed-off-by: Ban Tao 
> ---
>  .../bindings/pwm/pwm-sun8i-v536.txt   | 24 +++

Bindings should be done using the YAML format now to enable the DT
validation.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Maxime Ripard]

Hi,

On Tue, Mar 02, 2021 at 08:37:37PM +0800, Ban Tao wrote:
> From: Ban Tao 
> 
> The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> IP compared to the older Allwinner SoCs.
> 
> Signed-off-by: Ban Tao 

Thanks for sending an update.

Like I said in the previous version though, without proper SoC support
upstream, that driver would effectively be dead code

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] pinctrl/sunxi: adding input-debounce-ns property

[Maxime Ripard]

On Fri, Feb 26, 2021 at 01:53:00PM +0100, Marjan Pascolo wrote:
> Hi Maxime,
> 
> Il 17/02/2021 12:03, Maxime Ripard ha scritto:
> > Hi,
> > 
> > On Wed, Feb 10, 2021 at 05:22:37PM +0100, Marjan Pascolo wrote:
> > > On Allwinner SoC interrupt debounce can be controlled by two oscillator
> > > (32KHz and 24MHz) and a prescale divider.
> > > Oscillator and prescale divider are set through
> > > device tree property "input-debounce" which have 1uS accuracy.
> > > For acheive nS precision a new device tree poperty is made
> > > named "input-debounce-ns".
> > > "input-debounce-ns" is checked only if "input-debounce"
> > > property is not defined.
> > > 
> > > Suggested-by: Maxime Ripard 
> > > Signed-off-by: Marjan Pascolo 
> > > ---
> > > ---
> > >   .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  |  9 +++
> > >   drivers/pinctrl/sunxi/pinctrl-sunxi.c | 25 ---
> > >   2 files changed, 30 insertions(+), 4 deletions(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > index 5240487dfe50..346776de3a44 100644
> > > ---
> > > a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > +++
> > > b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> > > @@ -93,6 +93,15 @@ properties:
> > >   minItems: 1
> > >   maxItems: 5
> > > 
> > > +  input-debounce-ns:
> > > +    description:
> > > +  Debouncing periods in nanoseconds, one period per interrupt
> > > +  bank found in the controller.
> > > +  Only checked if input-debounce is not present
> > > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > +    minItems: 1
> > > +    maxItems: 5
> > > +
> > This should be a separate patch, with the DT maintainers in Cc.
> > 
> > You should enforce that the properties are mutually exclusive through
> > the schema too
> 
> I'm sorry, I've ignored documentaion about /Documentation.
> 
> I see that some additional YAML operator (like oneOf) are used.
> 
> oneOf should fit the schema, but I can't understand if oneOf's options must
> be a literal value, or if could also be a node.
> 
> Otherwise I'll use if ..then..else.

dependencies is what you're looking for, not oneOf or if

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support

[Maxime Ripard]

Hi,

On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote:
> On Mon, 1 Mar 2021 14:13:05 +0100
> Maxime Ripard  wrote:
> 
> > On Sat, Feb 27, 2021 at 02:03:54PM +0100, Wilken Gottwalt wrote:
> > > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> > > most of the sun6i compatible SoCs.
> > >
> > > This unit provides at least 32 spinlocks in hardware. The implementation
> > > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> > > reading a register and released by writing a 0 to it. This driver
> > > supports all 4 spinlock setups, but for now only the first setup (32
> > > locks) seem to exist in available devices. This spinlock unit is shared
> > > between all ARM cores and the embedded companion core. All of them can
> > > take/release a lock with a single cycle operation. It can be used to
> > > sync access to devices shared by the ARM cores and the companion core.
> > >
> > > There are two ways to check if a lock is taken. The first way is to read
> > > a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> > > is returned, the caller has to try again. Which means the lock is taken.
> > > The second way is to read a 32bit wide status register where every bit
> > > represents one of the 32 first locks. According to the datasheets this
> > > status register supports only the 32 first locks. This is the reason the
> > > first way (lock read/write) approach is used to be able to cover all 256
> > > locks in future devices. The driver also reports the amount of supported
> > > locks via debugfs.
> > >
> > > Signed-off-by: Wilken Gottwalt 
> 
> Nope, I had to replace the devm_hwspin_lock_register function by the
> hwspin_lock_register function because like Bjorn pointed out that it can
> fail and needs to handled correctly. And having a devm_* function does not
> play well with the non-devm clock/reset functions and winding back if an
> error occurs. It also messes with the call order in the remove function. So
> I went back to the classic way where I have full control over the call order.

If you're talking about the clock and reset line reassertion, I don't
really see what the trouble is. Sure, it's not going to be in the exact
same order in remove, but it's still going to execute in the proper
order (ie, clock disable, then reset disable, then clock put and reset
put). And you can use devm_add_action if you want to handle things
automatically.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v6 1/2] dt-bindings: hwlock: add sun6i_hwspinlock

[Maxime Ripard]

On Mon, Mar 01, 2021 at 03:06:35PM +0100, Wilken Gottwalt wrote:
> On Mon, 1 Mar 2021 14:12:44 +0100
> Maxime Ripard  wrote:
> 
> > On Sat, Feb 27, 2021 at 02:03:28PM +0100, Wilken Gottwalt wrote:
> > > Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> > > compatible series SoCs.
> > >
> > > Signed-off-by: Wilken Gottwalt 
> > > ---
> > > Changes in v6:
> > >   - fixed formating and name issues in dt documentation
> > >
> > > Changes in v5:
> > >   - changed binding to earliest known supported SoC sun6i-a31
> > >   - dropped unnecessary entries
> > >
> > > Changes in v4:
> > >   - changed binding to sun8i-a33-hwpinlock
> > >   - added changes suggested by Maxime Ripard
> > >
> > > Changes in v3:
> > >   - changed symbols from sunxi to sun8i
> > >
> > > Changes in v2:
> > >   - fixed memory ranges
> > > ---
> > >  .../hwlock/allwinner,sun6i-hwspinlock.yaml| 45 +++
> > 
> > The name of the file doesn't match the compatible, once fixed:
> > Acked-by: Maxime Ripard 
> 
> This is something that still confuses me. What if you have more than one
> compatible string?

In this case, it's fairly easy there's only one :)

But we're following the same rule than the compatible: the first SoC
that got the compatible wins 

> This won't be solvable. See the qcom binding for example,
> there are two strings and none matches. In the omap bindings are also two
> strings and only one matches. In all cases, including mine, the bindings
> check script is fine with that.

If other platforms want to follow other rules, good for them :)

> So, you basically want it to be called
> "allwinner,sun6i-a31-hwspinlock.yaml"?

Yes

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API

[Maxime Ripard]

On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote:
> On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard  wrote:
> >
> > Hi,
> >
> > On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
> > > Use drm_panel_bridge to replace manual panel handling code.
> > >
> > > This simplifies the driver to allows all components in the
> > > display pipeline to be treated as bridges, paving the way
> > > to generic connector handling.
> > >
> > > Use drm_bridge_connector_init to create a connector for display
> > > pipelines that use drm_bridge.
> > >
> > > This allows splitting connector operations across multiple bridges
> > > when necessary, instead of having the last bridge in the chain
> > > creating the connector and handling all connector operations
> > > internally.
> > >
> > > Signed-off-by: Jagan Teki 
> >
> > Most of the code removed in that patch was actually introduced earlier
> > which feels a bit weird. Is there a reason we can't do that one first,
> > and then introduce the bridge support?
> 
> This patch adds new bridge API's which requires the driver has to
> support the bridge first.

I'm not sure what you're saying, you can definitely have a bridge
without support for a downstream bridge.

Anyway, my point is that:

> ---
> Changes for v3:
> - new patch
>
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 +++--
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |   7 --
>  2 files changed, 27 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c 
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 3cdc14daf25c..5e5d3789b3df 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -20,6 +20,7 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -769,12 +770,6 @@ static void sun6i_dsi_bridge_pre_enable(struct 
> drm_bridge *bridge)
>   phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY);
>   phy_configure(dsi->dphy, );
>   phy_power_on(dsi->dphy);
> -
> - if (dsi->panel)
> - drm_panel_prepare(dsi->panel);
> -
> - if (dsi->panel_bridge)
> - dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);

This is added in patch 2

>  }
>
>  static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
> @@ -793,12 +788,6 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge 
> *bridge)
>* ordering on the panels I've tested it with, so I guess this
>* will do for now, until that IP is better understood.
>*/
> - if (dsi->panel)
> - drm_panel_enable(dsi->panel);
> -
> - if (dsi->panel_bridge)
> - dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
> -

This is added in patch 2

>   sun6i_dsi_start(dsi, DSI_START_HSC);
>
>   udelay(1000);
> @@ -812,14 +801,6 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge 
> *bridge)
>
>   DRM_DEBUG_DRIVER("Disabling DSI output\n");
>
> - if (dsi->panel) {
> - drm_panel_disable(dsi->panel);
> - drm_panel_unprepare(dsi->panel);
> - } else if (dsi->panel_bridge) {
> - dsi->panel_bridge->funcs->disable(dsi->panel_bridge);
> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
> - }
> -

This is added in patch 2

>   phy_power_off(dsi->dphy);
>   phy_exit(dsi->dphy);
>
> @@ -828,63 +809,13 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge 
> *bridge)
>   regulator_disable(dsi->regulator);
>  }
>
> -static int sun6i_dsi_get_modes(struct drm_connector *connector)
> -{
> - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> -
> - return drm_panel_get_modes(dsi->panel, connector);
> -}
> -
> -static const struct drm_connector_helper_funcs 
> sun6i_dsi_connector_helper_funcs = {
> - .get_modes  = sun6i_dsi_get_modes,
> -};
> -
> -static enum drm_connector_status
> -sun6i_dsi_connector_detect(struct drm_connector *connector, bool force)
> -{
> - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> -
> - return dsi->panel ? connector_status_connected :
> - connector_status_disconnected;
> -}
> -
> -static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
> - .detect = sun6i_dsi_connector_detect,
> - .fill_modes = drm_helper_probe_single_connector_modes,
> - .d

Re: [PATCH] arm64: dts: allwinner: h6: Switch to macros for RSB clock/reset indices

[Maxime Ripard]

Hi,

On Tue, Mar 02, 2021 at 12:23:09AM +0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai 
> 
> The macros for the clock and reset indices for the RSB hardware block
> were replaced with raw numbers when the RSB controller node was added.
> This was done to avoid cross-tree dependencies.
> 
> Now that both the clk and DT changes have been merged, we can switch
> back to using the macros.
> 
> Fixes: aaad900757a6 ("arm64: dts: allwinner: h6: Add RSB controller node")
> Signed-off-by: Chen-Yu Tsai 
> ---
> 
> Small patch split out from the H6 RSB clock support patch.
> This should be merged for v5.12.

It's not really a fix but something we can live with, so I've merged it
to sunxi/dt-for-5.13

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH 3/3] ARM: dts: sun8i: V3/S3: add i2s peripheral

[Maxime Ripard]

Hi,

On Fri, Feb 26, 2021 at 11:30:28AM +0100, Tobias Schramm wrote:
> The V3 and S3 SoCs feature an i2s peripheral identical to that of the H3.
> Add it to the dts.
> 
> Signed-off-by: Tobias Schramm 

Why is it added to the v3 DTSI and not the global one?

> ---
>  arch/arm/boot/dts/sun8i-v3.dtsi | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-v3.dtsi b/arch/arm/boot/dts/sun8i-v3.dtsi
> index c279e13583ba..17ea6b8f091f 100644
> --- a/arch/arm/boot/dts/sun8i-v3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-v3.dtsi
> @@ -30,3 +30,18 @@ uart1_pg_pins: uart1-pg-pins {
>   function = "uart1";
>   };
>  };
> +
> + {

You don't need a label for that, you can just put it under / { soc {

> + i2s0: i2s@1c22000 {
> + #sound-dai-cells = <0>;
> + compatible = "allwinner,sun8i-h3-i2s";

We should also have a v3 specific compatible here in addition to the H3
(and document it in the bindings).

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 1/3] ARM: dts: sun8i: V3s/V3/S3: add dma controller node

[Maxime Ripard]

On Fri, Feb 26, 2021 at 11:30:26AM +0100, Tobias Schramm wrote:
> The V3s, V3 and S3 SoCs have a dma controller. Add it to the dts.
> 
> Signed-off-by: Tobias Schramm 

Applied, thanks
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support

[Maxime Ripard]

On Sat, Feb 27, 2021 at 02:03:54PM +0100, Wilken Gottwalt wrote:
> Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> most of the sun6i compatible SoCs.
>
> This unit provides at least 32 spinlocks in hardware. The implementation
> supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> reading a register and released by writing a 0 to it. This driver
> supports all 4 spinlock setups, but for now only the first setup (32
> locks) seem to exist in available devices. This spinlock unit is shared
> between all ARM cores and the embedded companion core. All of them can
> take/release a lock with a single cycle operation. It can be used to
> sync access to devices shared by the ARM cores and the companion core.
>
> There are two ways to check if a lock is taken. The first way is to read
> a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> is returned, the caller has to try again. Which means the lock is taken.
> The second way is to read a 32bit wide status register where every bit
> represents one of the 32 first locks. According to the datasheets this
> status register supports only the 32 first locks. This is the reason the
> first way (lock read/write) approach is used to be able to cover all 256
> locks in future devices. The driver also reports the amount of supported
> locks via debugfs.
>
> Signed-off-by: Wilken Gottwalt 

Didn't I review this one already?
Maxime

Re: [PATCH v6 1/2] dt-bindings: hwlock: add sun6i_hwspinlock

[Maxime Ripard]

On Sat, Feb 27, 2021 at 02:03:28PM +0100, Wilken Gottwalt wrote:
> Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> compatible series SoCs.
>
> Signed-off-by: Wilken Gottwalt 
> ---
> Changes in v6:
>   - fixed formating and name issues in dt documentation
>
> Changes in v5:
>   - changed binding to earliest known supported SoC sun6i-a31
>   - dropped unnecessary entries
>
> Changes in v4:
>   - changed binding to sun8i-a33-hwpinlock
>   - added changes suggested by Maxime Ripard
>
> Changes in v3:
>   - changed symbols from sunxi to sun8i
>
> Changes in v2:
>   - fixed memory ranges
> ---
>  .../hwlock/allwinner,sun6i-hwspinlock.yaml| 45 +++

The name of the file doesn't match the compatible, once fixed:
Acked-by: Maxime Ripard 

Maxime

Re: [PATCH v3 6/7] drm: sun4i: dsi: Use drm_panel_bridge, connector API

[Maxime Ripard]

Hi,

On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
> Use drm_panel_bridge to replace manual panel handling code.
>
> This simplifies the driver to allows all components in the
> display pipeline to be treated as bridges, paving the way
> to generic connector handling.
>
> Use drm_bridge_connector_init to create a connector for display
> pipelines that use drm_bridge.
>
> This allows splitting connector operations across multiple bridges
> when necessary, instead of having the last bridge in the chain
> creating the connector and handling all connector operations
> internally.
>
> Signed-off-by: Jagan Teki 

Most of the code removed in that patch was actually introduced earlier
which feels a bit weird. Is there a reason we can't do that one first,
and then introduce the bridge support?

Maxime

Re: [PATCH v5 0/2] Add support for Topwise A721 tablet

[Maxime Ripard]

On Wed, Feb 24, 2021 at 11:52:38AM +0100, Pascal Roeleven wrote:
> On request I'm resending the last two patches from the Topwise A721 tablet
> series from a year ago as they weren't picked up. The other patches are
> already merged, so I didn't resend them.
> 
> Changes from v4:
> * Reorder nodes alphabetically
> 
> Changes from v3:
> * Fix DT validation warnings
> * Remove leftover labels
> 
> Changes from v2:
> * Collected acked-by.


Queued for 5.13, thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 01/11] drm/atomic: Pass the full state to planes async atomic check and update

[Maxime Ripard]

Hi,

On Wed, Feb 24, 2021 at 12:33:45PM +0100, Thomas Zimmermann wrote:
> Hi Maxime,
> 
> for the whole series:
> 
> Acked-by: Thomas Zimmermann 

Applied the whole series, thanks to everyone involved in the review,
it's been a pretty daunting one :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v4 2/2] ARM: dts: sun4i: Add support for Topwise A721 tablet

[Maxime Ripard]

Hi,

We're almost there, except for ...

On Mon, Feb 22, 2021 at 11:08:26AM +0100, Pascal Roeleven wrote:
> + {
> + vref-supply = <_ldo2>;
> + status = "okay";
> +
> + button-761 {
> + label = "Volume Down";
> + linux,code = ;
> + channel = <0>;
> + voltage = <761904>;
> + };
> +
> + button-571 {
> + label = "Volume Up";
> + linux,code = ;
> + channel = <0>;
> + voltage = <571428>;
> + };
> +};

Those nodes should be ordered alphabetically

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 06/11] drm: Use state helper instead of plane state pointer in atomic_check

[Maxime Ripard]

Hi Thomas,

On Mon, Feb 22, 2021 at 10:12:49AM +0100, Thomas Zimmermann wrote:
> Am 19.02.21 um 13:00 schrieb Maxime Ripard:
> > Many drivers reference the plane->state pointer in order to get the
> > current plane state in their atomic_check hook, which would be the old
> > plane state in the global atomic state since _swap_state hasn't happened
> > when atomic_check is run.
> > 
> > Use the drm_atomic_get_old_plane_state helper to get that state to make
> > it more obvious.
> > 
> > This was made using the coccinelle script below:
> > 
> > @ plane_atomic_func @
> > identifier helpers;
> > identifier func;
> > @@
> > 
> > static struct drm_plane_helper_funcs helpers = {
> > ...,
> > .atomic_check = func,
> > ...,
> > };
> > 
> > @ replaces_old_state @
> > identifier plane_atomic_func.func;
> > identifier plane, state, plane_state;
> > @@
> > 
> >   func(struct drm_plane *plane, struct drm_atomic_state *state) {
> > ...
> > -   struct drm_plane_state *plane_state = plane->state;
> > +   struct drm_plane_state *plane_state = 
> > drm_atomic_get_old_plane_state(state, plane);
> > ...
> >   }
> > 
> > @@
> > identifier plane_atomic_func.func;
> > identifier plane, state, plane_state;
> > @@
> > 
> >   func(struct drm_plane *plane, struct drm_atomic_state *state) {
> > struct drm_plane_state *plane_state = 
> > drm_atomic_get_old_plane_state(state, plane);
> > <...
> > -   plane->state
> > +   plane_state
> > ...>
> >   }
> > 
> > @ adds_old_state @
> > identifier plane_atomic_func.func;
> > identifier plane, state;
> > @@
> > 
> >   func(struct drm_plane *plane, struct drm_atomic_state *state) {
> > +   struct drm_plane_state *old_plane_state = 
> > drm_atomic_get_old_plane_state(state, plane);
> > <...
> > -   plane->state
> > +   old_plane_state
> > ...>
> >   }
> > 
> > @ include depends on adds_old_state || replaces_old_state @
> > @@
> > 
> >   #include 
> > 
> > @ no_include depends on !include && (adds_old_state || replaces_old_state) @
> > @@
> > 
> > + #include 
> >#include 
> > 
> > Reviewed-by: Ville Syrjälä 
> > Signed-off-by: Maxime Ripard 
> 
> Acked-by: Thomas Zimmermann 
> 
> However, I find 'old plane state' somewhat confusing in this context,
> because it's actually the current plane state. Would it make sense to use
> drm_atomic_get_existing_plane_state() instead?

drm_atomic_get_existing_plane_state is deprecated nowadays, in favour of either
drm_atomic_get_old_plane_state or drm_atomic_get_new_plane_state

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 02/11] drm: Rename plane atomic_check state names

[Maxime Ripard]

Hi Thomas,

Thanks for your review!

On Fri, Feb 19, 2021 at 03:49:22PM +0100, Thomas Zimmermann wrote:
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c 
> > b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index 075508051b5f..1873a155bb26 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -337,12 +337,12 @@ static const struct drm_plane_funcs ipu_plane_funcs = 
> > {
> >   };
> >   static int ipu_plane_atomic_check(struct drm_plane *plane,
> > - struct drm_plane_state *state)
> > + struct drm_plane_state *new_state)
> 
> This function uses a different naming convention then the others?
> 
> >   {
> > struct drm_plane_state *old_state = plane->state;

So, the function already had a variable named old_state, so I was
actually trying to make the drivers consistent here: having one variable
with old_state and new_plane_state felt weird.

The heuristic is thus to use the convention of the driver if one exists
already, and if there's none pick new_plane_state.

It makes it indeed inconsistent across drivers, but it felt more natural
to be consistent within a single driver.

Maxime


signature.asc
Description: PGP signature

[PATCH v3 10/11] drm: Use state helper instead of the plane state pointer

[Maxime Ripard]

Many drivers reference the plane->state pointer in order to get the
current plane state in their atomic_update or atomic_disable hooks,
which would be the new plane state in the global atomic state since
_swap_state happened when those hooks are run.

Use the drm_atomic_get_new_plane_state helper to get that state to make it
more obvious.

This was made using the coccinelle script below:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

(
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_disable = func,
...,
 };
|
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_update = func,
...,
 };
)

@ adds_new_state @
identifier plane_atomic_func.func;
identifier plane, state;
identifier new_state;
@@

 func(struct drm_plane *plane, struct drm_atomic_state *state)
 {
...
-   struct drm_plane_state *new_state = plane->state;
+   struct drm_plane_state *new_state = 
drm_atomic_get_new_plane_state(state, plane);
...
 }

@ include depends on adds_new_state @
@@

 #include 

@ no_include depends on !include && adds_new_state @
@@

+ #include 
  #include 

Reviewed-by: Ville Syrjälä 
Signed-off-by: Maxime Ripard 
---
 drivers/gpu/drm/arc/arcpgu_crtc.c   | 4 +++-
 drivers/gpu/drm/arm/hdlcd_crtc.c| 3 ++-
 drivers/gpu/drm/arm/malidp_planes.c | 3 ++-
 drivers/gpu/drm/armada/armada_overlay.c | 3 ++-
 drivers/gpu/drm/armada/armada_plane.c   | 3 ++-
 drivers/gpu/drm/ast/ast_mode.c  | 6 --
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 3 ++-
 drivers/gpu/drm/exynos/exynos_drm_plane.c   | 3 ++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 3 ++-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c  | 3 ++-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 3 ++-
 drivers/gpu/drm/imx/dcss/dcss-plane.c   | 3 ++-
 drivers/gpu/drm/imx/ipuv3-plane.c   | 3 ++-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c   | 3 ++-
 drivers/gpu/drm/ingenic/ingenic-ipu.c   | 3 ++-
 drivers/gpu/drm/kmb/kmb_plane.c | 3 ++-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c| 6 --
 drivers/gpu/drm/meson/meson_overlay.c   | 3 ++-
 drivers/gpu/drm/meson/meson_plane.c | 3 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 3 ++-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c  | 4 +++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c  | 3 ++-
 drivers/gpu/drm/mxsfb/mxsfb_kms.c   | 3 ++-
 drivers/gpu/drm/omapdrm/omap_plane.c| 6 --
 drivers/gpu/drm/qxl/qxl_display.c   | 6 --
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 3 ++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 3 ++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 ++-
 drivers/gpu/drm/sti/sti_cursor.c| 3 ++-
 drivers/gpu/drm/sti/sti_gdp.c   | 3 ++-
 drivers/gpu/drm/sti/sti_hqvdp.c | 3 ++-
 drivers/gpu/drm/stm/ltdc.c  | 3 ++-
 drivers/gpu/drm/sun4i/sun4i_layer.c | 3 ++-
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c  | 3 ++-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c  | 3 ++-
 drivers/gpu/drm/tegra/dc.c  | 6 --
 drivers/gpu/drm/tegra/hub.c | 3 ++-
 drivers/gpu/drm/tidss/tidss_plane.c | 3 ++-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c   | 3 ++-
 drivers/gpu/drm/vboxvideo/vbox_mode.c   | 6 --
 drivers/gpu/drm/vkms/vkms_plane.c   | 3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c| 3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c| 3 ++-
 drivers/gpu/drm/xlnx/zynqmp_disp.c  | 3 ++-
 drivers/gpu/drm/zte/zx_plane.c  | 6 --
 46 files changed, 108 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c 
b/drivers/gpu/drm/arc/arcpgu_crtc.c
index b185452d5542..7016f9cfe30d 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2016 Synopsys, Inc. (www.synopsys.com)
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -147,7 +148,8 @@ static const struct drm_crtc_helper_funcs 
arc_pgu_crtc_helper_funcs = {
 static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
 {
-   struct drm_plane_state *new_plane_state = plane->state;
+   struct drm_plane_state *new_plane_state = 
drm_atomic_get_new_plane_state(state,
+   
 plane);
struct arcpgu_drm_private *arcpgu;
struct drm_gem_cma_object *gem;
 
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 2500bf189420..7adb065169e9 100644
--- a/drivers/gpu/drm/arm/h

[PATCH v3 09/11] drm/atomic: Pass the full state to planes atomic disable and update

[Maxime Ripard]

The current atomic helpers have either their object state being passed as
an argument or the full atomic state.

The former is the pattern that was done at first, before switching to the
latter for new hooks or when it was needed.

Let's convert the remaining helpers to provide a consistent interface,
this time with the planes atomic_update and atomic_disable.

The conversion was done using the coccinelle script below, built tested on
all the drivers.

@@
identifier plane, plane_state;
symbol state;
@@

 struct drm_plane_helper_funcs {
...
void (*atomic_update)(struct drm_plane *plane,
- struct drm_plane_state *plane_state);
+ struct drm_atomic_state *state);
...
 }

@@
identifier plane, plane_state;
symbol state;
@@

 struct drm_plane_helper_funcs {
...
void (*atomic_disable)(struct drm_plane *plane,
-  struct drm_plane_state *plane_state);
+  struct drm_atomic_state *state);
...
 }

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

(
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_update = func,
...,
 };
|
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_disable = func,
...,
 };
)

@@
struct drm_plane_helper_funcs *FUNCS;
identifier f;
identifier crtc_state;
identifier plane, plane_state, state;
expression e;
@@

 f(struct drm_crtc_state *crtc_state)
 {
...
struct drm_atomic_state *state = e;
<+...
(
-   FUNCS->atomic_disable(plane, plane_state)
+   FUNCS->atomic_disable(plane, state)
|
-   FUNCS->atomic_update(plane, plane_state)
+   FUNCS->atomic_update(plane, state)
)
...+>
 }

@@
identifier plane_atomic_func.func;
identifier plane;
symbol state;
@@

 func(struct drm_plane *plane,
-struct drm_plane_state *state)
+struct drm_plane_state *old_plane_state)
 {
<...
-   state
+   old_plane_state
...>
 }

@ ignores_old_state @
identifier plane_atomic_func.func;
identifier plane, old_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_state)
 {
... when != old_state
 }

@ adds_old_state depends on plane_atomic_func && !ignores_old_state @
identifier plane_atomic_func.func;
identifier plane, plane_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *plane_state)
 {
+   struct drm_plane_state *plane_state = 
drm_atomic_get_old_plane_state(state, plane);
...
 }

@ depends on plane_atomic_func @
identifier plane_atomic_func.func;
identifier plane, plane_state;
@@

 func(struct drm_plane *plane,
- struct drm_plane_state *plane_state
+ struct drm_atomic_state *state
 )
 { ... }

@ include depends on adds_old_state @
@@

 #include 

@ no_include depends on !include && adds_old_state @
@@

+ #include 
  #include 

@@
identifier plane_atomic_func.func;
identifier plane, state;
identifier plane_state;
@@

 func(struct drm_plane *plane, struct drm_atomic_state *state) {
...
struct drm_plane_state *plane_state = 
drm_atomic_get_old_plane_state(state, plane);
<+...
-   plane_state->state
+   state
...+>
 }

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Reintroduce the old_plane_state check in zynqmp_disp_crtc_atomic_disable
---
 drivers/gpu/drm/arc/arcpgu_crtc.c |  2 +-
 drivers/gpu/drm/arm/display/komeda/komeda_plane.c |  2 +-
 drivers/gpu/drm/arm/hdlcd_crtc.c  |  2 +-
 drivers/gpu/drm/arm/malidp_planes.c   |  6 --
 drivers/gpu/drm/armada/armada_overlay.c   |  8 ++--
 drivers/gpu/drm/armada/armada_plane.c |  8 ++--
 drivers/gpu/drm/ast/ast_mode.c| 12 
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   |  6 +++---
 drivers/gpu/drm/drm_atomic_helper.c   |  8 
 drivers/gpu/drm/drm_simple_kms_helper.c   |  4 +++-
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  6 --
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c   |  4 ++--
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c|  2 +-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c   |  4 ++--
 drivers/gpu/drm/imx/dcss/dcss-plane.c |  6 --
 drivers/gpu/drm/imx/ipuv3-plane.c |  6 --
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c |  4 ++--
 drivers/gpu/drm/ingenic/ingenic-ipu.c |  4 ++--
 drivers/gpu/drm/kmb/kmb_plane.c   |  8 +---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c   |  4 ++--
 drivers/gpu/drm/mediatek/mtk_drm_crtc.h   |  2 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  |  8 
 drivers/gpu/drm/meson/meson_overlay.c |  4 ++--
 drivers/gpu/drm/meson/meson_plane.c   | 

[PATCH v3 07/11] drm: Store new plane state in a variable for atomic_update and disable

[Maxime Ripard]

In order to store the new plane state in a subsequent helper, let's move
the plane->state dereferences into a variable.

This was done using the following coccinelle script, plus some hand
changes for vmwgfx:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

(
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_disable = func,
...,
 };
|
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_update = func,
...,
 };
)

@ has_new_state_old_state @
identifier plane_atomic_func.func;
identifier plane;
identifier new_state;
symbol old_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_state)
 {
...
struct drm_plane_state *new_state = plane->state;
...
 }

@ depends on !has_new_state_old_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_state)
 {
+   struct drm_plane_state *new_state = plane->state;
<+...
-   plane->state
+   new_state
...+>
 }

@ has_new_state_state @
identifier plane_atomic_func.func;
identifier plane;
identifier new_state;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *state)
 {
...
struct drm_plane_state *new_state = plane->state;
...
 }

@ depends on !has_new_state_state @
identifier plane_atomic_func.func;
identifier plane;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *state)
 {
+   struct drm_plane_state *new_plane_state = plane->state;
<+...
-   plane->state
+   new_plane_state
...+>
 }

@ has_new_state_old_s @
identifier plane_atomic_func.func;
identifier plane;
identifier new_state;
symbol old_s;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_s)
 {
...
struct drm_plane_state *new_state = plane->state;
...
 }

@ depends on !has_new_state_old_s @
identifier plane_atomic_func.func;
identifier plane;
symbol old_s;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_s)
 {
+   struct drm_plane_state *new_s = plane->state;
<+...
-   plane->state
+   new_s
...+>
 }

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Wrapping change suggested by Laurent in omapdrm
---
 drivers/gpu/drm/arc/arcpgu_crtc.c |  7 ++--
 drivers/gpu/drm/arm/hdlcd_crtc.c  |  7 ++--
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   |  5 ++-
 drivers/gpu/drm/kmb/kmb_plane.c   | 19 +
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 26 ++--
 drivers/gpu/drm/omapdrm/omap_plane.c  |  6 +--
 drivers/gpu/drm/qxl/qxl_display.c | 20 +
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  5 ++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c |  3 +-
 drivers/gpu/drm/sun4i/sun4i_layer.c   |  3 +-
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c|  5 ++-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c|  5 ++-
 drivers/gpu/drm/tegra/dc.c| 42 ++-
 drivers/gpu/drm/tegra/hub.c   | 25 +--
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 24 ++-
 drivers/gpu/drm/vkms/vkms_plane.c | 11 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 19 +
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c   |  5 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c  |  7 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c  |  9 ++--
 drivers/gpu/drm/xlnx/zynqmp_disp.c|  7 ++--
 drivers/gpu/drm/zte/zx_plane.c| 19 +
 22 files changed, 152 insertions(+), 127 deletions(-)

diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c 
b/drivers/gpu/drm/arc/arcpgu_crtc.c
index 895cdd991af6..2cea17a96d5c 100644
--- a/drivers/gpu/drm/arc/arcpgu_crtc.c
+++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
@@ -147,14 +147,15 @@ static const struct drm_crtc_helper_funcs 
arc_pgu_crtc_helper_funcs = {
 static void arc_pgu_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *state)
 {
+   struct drm_plane_state *new_plane_state = plane->state;
struct arcpgu_drm_private *arcpgu;
struct drm_gem_cma_object *gem;
 
-   if (!plane->state->crtc || !plane->state->fb)
+   if (!new_plane_state->crtc || !new_plane_state->fb)
return;
 
-   arcpgu = crtc_to_arcpgu_priv(plane->state->crtc);
-   gem = drm_fb_cma_get_gem_obj(plane->state->fb, 0);
+   arcpgu = crtc_to_arcpgu_priv(new_plane_state->crtc);
+   gem = drm_fb_cma_get_gem_obj(new_plane_state->fb, 0);
arc_pgu_write(arcpgu, ARCPGU_REG_BUF0_ADDR, gem->paddr);
 }
 
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 028ec39c8484..3f050a52e07a 1006

Re: [PATCH v2 1/1] clk: sunxi-ng: v3s: use sigma-delta modulation for audio-pll

[Maxime Ripard]

On Thu, Feb 18, 2021 at 12:20:01PM +0100, Tobias Schramm wrote:
> Previously it was not possible to achieve clock rates of 24.576MHz and
> 22.5792MHz, which are commonly required core clocks for the i2s
> peripheral of v3s based SoCs.
> 
> Add support for those clock rates through the audio pll's sigma-delta
> modulator.
> 
> Signed-off-by: Tobias Schramm 

Applied, thanks
Maxime


signature.asc
Description: PGP signature

[PATCH v3 05/11] drm: Use the state pointer directly in planes atomic_check

[Maxime Ripard]

Now that atomic_check takes the global atomic state as a parameter, we
don't need to go through the pointer in the plane state.

This was done using the following coccinelle script:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

static struct drm_plane_helper_funcs helpers = {
...,
.atomic_check = func,
...,
};

@@
identifier plane_atomic_func.func;
identifier plane, state;
identifier plane_state;
@@

  func(struct drm_plane *plane, struct drm_atomic_state *state) {
  ...
- struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, 
plane);
  <... when != plane_state
- plane_state->state
+ state
  ...>
 }

@@
identifier plane_atomic_func.func;
identifier plane, state;
identifier plane_state;
@@

  func(struct drm_plane *plane, struct drm_atomic_state *state) {
  ...
  struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, 
plane);
  <...
- plane_state->state
+ state
  ...>
 }

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Fixed the formatting in zynqmp_disp
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
 drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 2 +-
 drivers/gpu/drm/arm/hdlcd_crtc.c  | 2 +-
 drivers/gpu/drm/armada/armada_plane.c | 4 ++--
 drivers/gpu/drm/ast/ast_mode.c| 4 ++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   | 2 +-
 drivers/gpu/drm/drm_simple_kms_helper.c   | 2 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 2 +-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c   | 2 +-
 drivers/gpu/drm/imx/dcss/dcss-plane.c | 2 +-
 drivers/gpu/drm/imx/ipuv3-plane.c | 2 +-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +-
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 2 +-
 drivers/gpu/drm/kmb/kmb_plane.c   | 2 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 2 +-
 drivers/gpu/drm/meson/meson_overlay.c | 2 +-
 drivers/gpu/drm/meson/meson_plane.c   | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 2 +-
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 +-
 drivers/gpu/drm/omapdrm/omap_plane.c  | 2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 2 +-
 drivers/gpu/drm/sti/sti_cursor.c  | 2 +-
 drivers/gpu/drm/sti/sti_gdp.c | 2 +-
 drivers/gpu/drm/sti/sti_hqvdp.c   | 2 +-
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c| 2 +-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c| 2 +-
 drivers/gpu/drm/tidss/tidss_plane.c   | 2 +-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 2 +-
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 8 
 drivers/gpu/drm/virtio/virtgpu_plane.c| 2 +-
 drivers/gpu/drm/vkms/vkms_plane.c | 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 2 +-
 drivers/gpu/drm/xlnx/zynqmp_disp.c| 3 +--
 drivers/gpu/drm/zte/zx_plane.c| 4 ++--
 35 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1cdff048b0c0..22124f76d0b5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6451,7 +6451,7 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
return 0;
 
new_crtc_state =
-   drm_atomic_get_new_crtc_state(new_plane_state->state,
+   drm_atomic_get_new_crtc_state(state,
  new_plane_state->crtc);
if (!new_crtc_state)
return -EINVAL;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c 
b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index 96a6fe95a4e7..13582c174bbb 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -84,7 +84,7 @@ komeda_plane_atomic_check(struct drm_plane *plane,
if (!new_plane_state->crtc || !new_plane_state->fb)
return 0;
 
-   crtc_st = drm_atomic_get_crtc_state(new_plane_state->state,
+   crtc_st = drm_atomic_get_crtc_state(state,
new_plane_state->crtc);
if (IS_ERR(crtc_st) || !crtc_st->enable) {
DRM_DEBUG_ATOMIC("Cannot update plane on a disabled CRTC.\n");
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 9da9d0581ce9..028ec39c8484 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -244,7 +244,7 @@ static int hdlcd_plane_atomic_check(struct drm_plane *plane,
return -EINVAL;
}
 
-   for_each_new_crtc_

[PATCH v3 11/11] drm/todo: Remove the drm_atomic_state todo item

[Maxime Ripard]

Only planes' prepare_fb and cleanup_fb, and encoders' atomic_check and
atomic_mode_set hooks remain with an object state and not the global
drm_atomic_state.

prepare_fb and cleanup_fb operate by design on a given state and
depending on the calling site can operate on either the old or new
state, so it doesn't really make much sense to convert them.

The encoders' atomic_check and atomic_mode_set operate on the CRTC and
connector state connected to them since encoders don't have a state of
their own. Without those state pointers, we would need to get the CRTC
through the drm_connector_state crtc pointer.

However, in order to get the drm_connector_state pointer, we would need
to get the connector itself and while usually we have a single connector
connected to the encoder, we can't really get it from the encoder at
the moment since it could be behind any number of bridges.

While this could be addressed by (for example) listing all the
connectors and finding the one that has the encoder as its source, it
feels like an unnecessary rework for something that is slowly getting
replaced by bridges.

Since all the users that matter have been converted, let's remove the
TODO item.

Acked-by: Daniel Vetter 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - New patch
---
 Documentation/gpu/todo.rst | 46 --
 1 file changed, 46 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index f872d3d33218..0631b9b323d5 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -459,52 +459,6 @@ Contact: Emil Velikov, respective driver maintainers
 
 Level: Intermediate
 
-Plumb drm_atomic_state all over

-
-Currently various atomic functions take just a single or a handful of
-object states (eg. plane state). While that single object state can
-suffice for some simple cases, we often have to dig out additional
-object states for dealing with various dependencies between the individual
-objects or the hardware they represent. The process of digging out the
-additional states is rather non-intuitive and error prone.
-
-To fix that most functions should rather take the overall
-drm_atomic_state as one of their parameters. The other parameters
-would generally be the object(s) we mainly want to interact with.
-
-For example, instead of
-
-.. code-block:: c
-
-   int (*atomic_check)(struct drm_plane *plane, struct drm_plane_state *state);
-
-we would have something like
-
-.. code-block:: c
-
-   int (*atomic_check)(struct drm_plane *plane, struct drm_atomic_state 
*state);
-
-The implementation can then trivially gain access to any required object
-state(s) via drm_atomic_get_plane_state(), drm_atomic_get_new_plane_state(),
-drm_atomic_get_old_plane_state(), and their equivalents for
-other object types.
-
-Additionally many drivers currently access the object->state pointer
-directly in their commit functions. That is not going to work if we
-eg. want to allow deeper commit pipelines as those pointers could
-then point to the states corresponding to a future commit instead of
-the current commit we're trying to process. Also non-blocking commits
-execute locklessly so there are serious concerns with dereferencing
-the object->state pointers without holding the locks that protect them.
-Use of drm_atomic_get_new_plane_state(), drm_atomic_get_old_plane_state(),
-etc. avoids these problems as well since they relate to a specific
-commit via the passed in drm_atomic_state.
-
-Contact: Ville Syrjälä, Daniel Vetter
-
-Level: Intermediate
-
 Use struct dma_buf_map throughout codebase
 --
 
-- 
2.29.2

[PATCH v3 04/11] drm/atomic: Pass the full state to planes atomic_check

[Maxime Ripard]

The current atomic helpers have either their object state being passed as
an argument or the full atomic state.

The former is the pattern that was done at first, before switching to the
latter for new hooks or when it was needed.

Let's convert all the remaining helpers to provide a consistent
interface, starting with the planes atomic_check.

The conversion was done using the coccinelle script below plus some
manual changes for vmwgfx, built tested on all the drivers.

@@
identifier plane, plane_state;
symbol state;
@@

 struct drm_plane_helper_funcs {
...
int (*atomic_check)(struct drm_plane *plane,
-   struct drm_plane_state *plane_state);
+   struct drm_atomic_state *state);
...
}

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_check = func,
...,
};

@@
struct drm_plane_helper_funcs *FUNCS;
identifier f;
identifier dev;
identifier plane, plane_state, state;
@@

 f(struct drm_device *dev, struct drm_atomic_state *state)
 {
<+...
-   FUNCS->atomic_check(plane, plane_state)
+   FUNCS->atomic_check(plane, state)
...+>
 }

@ ignores_new_state @
identifier plane_atomic_func.func;
identifier plane, new_plane_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *new_plane_state)
 {
... when != new_plane_state
 }

@ adds_new_state depends on plane_atomic_func && !ignores_new_state @
identifier plane_atomic_func.func;
identifier plane, new_plane_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *new_plane_state)
 {
+   struct drm_plane_state *new_plane_state = 
drm_atomic_get_new_plane_state(state, plane);
...
 }

@ depends on plane_atomic_func @
identifier plane_atomic_func.func;
identifier plane, new_plane_state;
@@

 func(struct drm_plane *plane,
- struct drm_plane_state *new_plane_state
+ struct drm_atomic_state *state
 )
 { ... }

@ include depends on adds_new_state @
@@

 #include 

@ no_include depends on !include && adds_new_state @
@@

+ #include 
  #include 

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Rewording and removal of a coccinelle rule suggested by Laurent
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +++-
 drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 4 +++-
 drivers/gpu/drm/arm/hdlcd_crtc.c  | 4 +++-
 drivers/gpu/drm/arm/malidp_planes.c   | 4 +++-
 drivers/gpu/drm/armada/armada_plane.c | 4 +++-
 drivers/gpu/drm/armada/armada_plane.h | 2 +-
 drivers/gpu/drm/ast/ast_mode.c| 8 ++--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   | 3 ++-
 drivers/gpu/drm/drm_atomic_helper.c   | 2 +-
 drivers/gpu/drm/drm_simple_kms_helper.c   | 4 +++-
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +++-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c   | 5 -
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 4 +++-
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c   | 4 +++-
 drivers/gpu/drm/imx/dcss/dcss-plane.c | 4 +++-
 drivers/gpu/drm/imx/ipuv3-plane.c | 4 +++-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 +++-
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 4 +++-
 drivers/gpu/drm/kmb/kmb_plane.c   | 4 +++-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 4 +++-
 drivers/gpu/drm/meson/meson_overlay.c | 4 +++-
 drivers/gpu/drm/meson/meson_plane.c   | 4 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 5 -
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c| 2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 4 +++-
 drivers/gpu/drm/mxsfb/mxsfb_kms.c | 4 +++-
 drivers/gpu/drm/nouveau/dispnv50/wndw.c   | 5 -
 drivers/gpu/drm/omapdrm/omap_plane.c  | 4 +++-
 drivers/gpu/drm/qxl/qxl_display.c | 4 +++-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   | 4 +++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 5 -
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 4 +++-
 drivers/gpu/drm/sti/sti_cursor.c  | 4 +++-
 drivers/gpu/drm/sti/sti_gdp.c | 4 +++-
 drivers/gpu/drm/sti/sti_hqvdp.c   | 4 +++-
 drivers/gpu/drm/stm/ltdc.c| 4 +++-
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c| 4 +++-
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c| 4 +++-
 drivers/gpu/drm/tegra/dc.c| 8 ++--
 drivers/gpu/drm/tegra/hub.c   | 4 +++-
 drivers/gpu/drm/tidss/tidss_plane.c   | 4 +++-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 4 +++-
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 8 ++--
 drivers/gpu/drm/vc4/vc4_plane.c   | 4 

[PATCH v3 02/11] drm: Rename plane atomic_check state names

[Maxime Ripard]

Most drivers call the argument to the plane atomic_check hook simply
state, which is going to conflict with the global atomic state in a
later rework. Let's rename it to new_plane_state (or new_state depending
on the convention used in the driver).

This was done using the coccinelle script below, and built tested:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

 static const struct drm_plane_helper_funcs helpers = {
.atomic_check = func,
 };

@ has_old_state @
identifier plane_atomic_func.func;
identifier plane;
expression e;
symbol old_state;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *state)
 {
...
struct drm_plane_state *old_state = e;
...
 }

@ depends on has_old_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

 func(struct drm_plane *plane,
-   struct drm_plane_state *state
+   struct drm_plane_state *new_state
 )
 {
<+...
-   state
+   new_state
...+>
 }

@ has_state @
identifier plane_atomic_func.func;
identifier plane;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *state)
 {
...
 }

@ depends on has_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
@@

 func(struct drm_plane *plane,
-   struct drm_plane_state *state
+   struct drm_plane_state *new_plane_state
 )
 {
<+...
-   state
+   new_plane_state
...+>
 }

Reviewed-by: Laurent Pinchart 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Updated the variable name in the comment in omapdrm
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++---
 .../gpu/drm/arm/display/komeda/komeda_plane.c | 11 ++---
 drivers/gpu/drm/arm/hdlcd_crtc.c  | 18 
 drivers/gpu/drm/arm/malidp_planes.c   | 36 
 drivers/gpu/drm/armada/armada_plane.c | 41 ++-
 drivers/gpu/drm/ast/ast_mode.c| 26 ++--
 drivers/gpu/drm/exynos/exynos_drm_plane.c |  6 +--
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c   |  6 +--
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 22 +-
 .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c   | 24 +--
 drivers/gpu/drm/imx/dcss/dcss-plane.c | 26 ++--
 drivers/gpu/drm/imx/ipuv3-plane.c | 31 +++---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 27 ++--
 drivers/gpu/drm/ingenic/ingenic-ipu.c | 30 +++---
 drivers/gpu/drm/kmb/kmb_plane.c   | 22 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 16 
 drivers/gpu/drm/meson/meson_overlay.c | 10 +++--
 drivers/gpu/drm/meson/meson_plane.c   | 10 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|  9 ++--
 drivers/gpu/drm/nouveau/dispnv50/wndw.c   |  5 ++-
 drivers/gpu/drm/omapdrm/omap_plane.c  | 21 +-
 drivers/gpu/drm/qxl/qxl_display.c |  6 +--
 drivers/gpu/drm/rcar-du/rcar_du_plane.c   |  7 ++--
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c |  7 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 27 ++--
 drivers/gpu/drm/sti/sti_cursor.c  | 22 +-
 drivers/gpu/drm/sti/sti_gdp.c | 26 ++--
 drivers/gpu/drm/sti/sti_hqvdp.c   | 24 +--
 drivers/gpu/drm/stm/ltdc.c| 10 ++---
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c| 10 +++--
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c| 10 +++--
 drivers/gpu/drm/tegra/dc.c| 38 -
 drivers/gpu/drm/tegra/hub.c   | 18 
 drivers/gpu/drm/tidss/tidss_plane.c   | 34 ---
 drivers/gpu/drm/tilcdc/tilcdc_plane.c | 24 +--
 drivers/gpu/drm/vc4/vc4_plane.c   | 10 ++---
 drivers/gpu/drm/virtio/virtgpu_plane.c|  9 ++--
 drivers/gpu/drm/vkms/vkms_plane.c | 11 ++---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   | 13 +++---
 drivers/gpu/drm/xlnx/zynqmp_disp.c| 10 +++--
 41 files changed, 403 insertions(+), 358 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 63f839679a0a..906fa4ae25c9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6432,7 +6432,7 @@ static int dm_plane_helper_check_state(struct 
drm_plane_state *state,
 }
 
 static int dm_plane_atomic_check(struct drm_plane *plane,
-struct drm_plane_state *state)
+struct drm_plane_state *new_plane_state)
 {
struct amdgpu_device *adev = drm_to_adev(plane->dev);
struct dc *dc = adev->dm.dc;
@@ -6441,23 +6441,24 @@ static int dm_plane_atomic_check(struct drm_plane 
*plane,
str

[PATCH v3 08/11] drm: Rename plane->state variables in atomic update and disable

[Maxime Ripard]

Some drivers are storing the plane->state pointer in atomic_update and
atomic_disable in a variable simply called state, while the state passed
as an argument is called old_state.

In order to ease subsequent reworks and to avoid confusing or
inconsistent names, let's rename those variables to new_state.

This was done using the following coccinelle script, plus some manual
changes for mtk and tegra.

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

(
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_disable = func,
...,
 };
|
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_update = func,
...,
 };
)

@ moves_new_state_old_state @
identifier plane_atomic_func.func;
identifier plane;
symbol old_state;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_state)
 {
...
-   struct drm_plane_state *state = plane->state;
+   struct drm_plane_state *new_state = plane->state;
...
 }

@ depends on moves_new_state_old_state @
identifier plane_atomic_func.func;
identifier plane;
identifier old_state;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_state)
 {
<...
-   state
+   new_state
...>
 }

@ moves_new_state_oldstate @
identifier plane_atomic_func.func;
identifier plane;
symbol oldstate;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *oldstate)
 {
...
-   struct drm_plane_state *state = plane->state;
+   struct drm_plane_state *newstate = plane->state;
...
 }

@ depends on moves_new_state_oldstate @
identifier plane_atomic_func.func;
identifier plane;
identifier old_state;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_state)
 {
<...
-   state
+   newstate
...>
 }

@ moves_new_state_old_pstate @
identifier plane_atomic_func.func;
identifier plane;
symbol old_pstate;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_pstate)
 {
...
-   struct drm_plane_state *state = plane->state;
+   struct drm_plane_state *new_pstate = plane->state;
...
 }

@ depends on moves_new_state_old_pstate @
identifier plane_atomic_func.func;
identifier plane;
identifier old_pstate;
symbol state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *old_pstate)
 {
<...
-   state
+   new_pstate
...>
 }

Reviewed-by: Ville Syrjälä 
Signed-off-by: Maxime Ripard 
---
 drivers/gpu/drm/arm/malidp_planes.c   |  34 +++---
 drivers/gpu/drm/armada/armada_overlay.c   | 104 +-
 drivers/gpu/drm/armada/armada_plane.c |  63 +--
 drivers/gpu/drm/ast/ast_mode.c|  30 ++---
 drivers/gpu/drm/exynos/exynos_drm_plane.c |   6 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c   |  10 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c|  12 +-
 .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c   |  11 +-
 drivers/gpu/drm/imx/dcss/dcss-plane.c |  25 +++--
 drivers/gpu/drm/imx/ipuv3-plane.c |  45 
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c |  16 +--
 drivers/gpu/drm/ingenic/ingenic-ipu.c |  34 +++---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  |  29 +++--
 drivers/gpu/drm/meson/meson_overlay.c |   6 +-
 drivers/gpu/drm/meson/meson_plane.c   |  30 ++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c |   4 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c|  12 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c|   8 +-
 drivers/gpu/drm/mxsfb/mxsfb_kms.c |   4 +-
 drivers/gpu/drm/omapdrm/omap_plane.c  |  21 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  20 ++--
 drivers/gpu/drm/sti/sti_cursor.c  |  10 +-
 drivers/gpu/drm/sti/sti_gdp.c |  40 +++
 drivers/gpu/drm/sti/sti_hqvdp.c   |  40 +++
 drivers/gpu/drm/stm/ltdc.c|  28 ++---
 drivers/gpu/drm/tegra/dc.c|  20 ++--
 drivers/gpu/drm/tegra/hub.c   |  10 +-
 drivers/gpu/drm/tidss/tidss_plane.c   |   8 +-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c |  14 +--
 drivers/gpu/drm/zte/zx_plane.c|   8 +-
 30 files changed, 355 insertions(+), 347 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_planes.c 
b/drivers/gpu/drm/arm/malidp_planes.c
index c94c4a96db68..646b27a42452 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -795,9 +795,9 @@ static void malidp_de_plane_update(struct drm_plane *plane,
 {
struct malidp_plane *mp;
struct malidp_plane_state *ms = to_malidp_plane_state(plane->state);
-   struct drm_plane_state *state = plane->state;
-   u16 pixel_alpha = state->pixel_blend_mode;
-   u8 plane_alpha = state->alpha >> 8;
+   struct drm_plane

[PATCH v3 01/11] drm/atomic: Pass the full state to planes async atomic check and update

[Maxime Ripard]

The current atomic helpers have either their object state being passed as
an argument or the full atomic state.

The former is the pattern that was done at first, before switching to the
latter for new hooks or when it was needed.

Let's start convert all the remaining helpers to provide a consistent
interface, starting with the planes atomic_async_check and
atomic_async_update.

The conversion was done using the coccinelle script below, built tested on
all the drivers.

@@
identifier plane, plane_state;
symbol state;
@@

 struct drm_plane_helper_funcs {
...
int (*atomic_async_check)(struct drm_plane *plane,
- struct drm_plane_state *plane_state);
+ struct drm_atomic_state *state);
...
 }

@@
identifier plane, plane_state;
symbol state;
@@
 struct drm_plane_helper_funcs {
...
void (*atomic_async_update)(struct drm_plane *plane,
-   struct drm_plane_state *plane_state);
+   struct drm_atomic_state *state);
...
 }

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

(
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_async_check = func,
...,
 };
|
 static const struct drm_plane_helper_funcs helpers = {
...,
.atomic_async_update = func,
...,
 };
)

@@
struct drm_plane_helper_funcs *FUNCS;
identifier f;
identifier dev;
identifier plane, plane_state, state;
@@

 f(struct drm_device *dev, struct drm_atomic_state *state)
 {
<+...
-   FUNCS->atomic_async_check(plane, plane_state)
+   FUNCS->atomic_async_check(plane, state)
...+>
 }

@@
struct drm_plane_helper_funcs *FUNCS;
identifier f;
identifier dev;
identifier plane, plane_state, state;
@@

 f(struct drm_device *dev, struct drm_atomic_state *state)
 {
<+...
-   FUNCS->atomic_async_update(plane, plane_state)
+   FUNCS->atomic_async_update(plane, state)
...+>
 }

@@
identifier mtk_plane_atomic_async_update;
identifier plane;
symbol new_state, state;
expression e;
@@

  void mtk_plane_atomic_async_update(struct drm_plane *plane, struct 
drm_plane_state *new_state)
{
  ...
- struct mtk_plane_state *state = e;
+ struct mtk_plane_state *new_plane_state = e;
  <+...
-   state
+   new_plane_state
  ...+>
  }

@@
identifier plane_atomic_func.func;
identifier plane;
symbol state;
@@

 func(struct drm_plane *plane,
-struct drm_plane_state *state)
+struct drm_plane_state *new_plane_state)
 {
<...
-   state
+   new_plane_state
...>
 }

@ ignores_new_state @
identifier plane_atomic_func.func;
identifier plane, new_plane_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *new_plane_state)
 {
... when != new_plane_state
 }

@ adds_new_state depends on plane_atomic_func && !ignores_new_state @
identifier plane_atomic_func.func;
identifier plane, new_plane_state;
@@

 func(struct drm_plane *plane, struct drm_plane_state *new_plane_state)
 {
+   struct drm_plane_state *new_plane_state = 
drm_atomic_get_new_plane_state(state, plane);
...
 }

@ depends on plane_atomic_func @
identifier plane_atomic_func.func;
identifier plane, plane_state;
@@

 func(struct drm_plane *plane,
- struct drm_plane_state *plane_state
+ struct drm_atomic_state *state
 )
 { ... }

@ include depends on adds_new_state @
@@

 #include 

@ no_include depends on !include && adds_new_state @
@@

+ #include 
  #include 

@@
identifier plane_atomic_func.func;
identifier plane, state;
identifier plane_state;
@@

 func(struct drm_plane *plane, struct drm_atomic_state *state) {
...
struct drm_plane_state *plane_state = 
drm_atomic_get_new_plane_state(state, plane);
<+...
-   plane_state->state
+   state
...+>
 }

Acked-by: Thomas Zimmermann 
Signed-off-by: Maxime Ripard 

---

Changes from v1:
  - Updated the comment according to Thomas suggestions
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++-
 drivers/gpu/drm/drm_atomic_helper.c   |  4 +-
 drivers/gpu/drm/mediatek/mtk_drm_plane.c  | 26 +
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c| 33 ++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   | 16 --
 drivers/gpu/drm/vc4/vc4_plane.c   | 56 ++-
 include/drm/drm_modeset_helper_vtables.h  | 18 +++---
 7 files changed, 89 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 6ed96633425f..63f839679a0a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6468,7 +6468,7 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
 }
 
 static int dm_plane_atomic_async_check(struct drm_plane *plane,

[PATCH v3 06/11] drm: Use state helper instead of plane state pointer in atomic_check

[Maxime Ripard]

Many drivers reference the plane->state pointer in order to get the
current plane state in their atomic_check hook, which would be the old
plane state in the global atomic state since _swap_state hasn't happened
when atomic_check is run.

Use the drm_atomic_get_old_plane_state helper to get that state to make
it more obvious.

This was made using the coccinelle script below:

@ plane_atomic_func @
identifier helpers;
identifier func;
@@

static struct drm_plane_helper_funcs helpers = {
...,
.atomic_check = func,
...,
};

@ replaces_old_state @
identifier plane_atomic_func.func;
identifier plane, state, plane_state;
@@

 func(struct drm_plane *plane, struct drm_atomic_state *state) {
...
-   struct drm_plane_state *plane_state = plane->state;
+   struct drm_plane_state *plane_state = 
drm_atomic_get_old_plane_state(state, plane);
...
 }

@@
identifier plane_atomic_func.func;
identifier plane, state, plane_state;
@@

 func(struct drm_plane *plane, struct drm_atomic_state *state) {
struct drm_plane_state *plane_state = 
drm_atomic_get_old_plane_state(state, plane);
<...
-   plane->state
+   plane_state
...>
 }

@ adds_old_state @
identifier plane_atomic_func.func;
identifier plane, state;
@@

 func(struct drm_plane *plane, struct drm_atomic_state *state) {
+   struct drm_plane_state *old_plane_state = 
drm_atomic_get_old_plane_state(state, plane);
<...
-   plane->state
+   old_plane_state
...>
 }

@ include depends on adds_old_state || replaces_old_state @
@@

 #include 

@ no_include depends on !include && (adds_old_state || replaces_old_state) @
@@

+ #include 
  #include 

Reviewed-by: Ville Syrjälä 
Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - s/.../<.../ in the coccinelle script as suggested by Ville
---
 drivers/gpu/drm/imx/ipuv3-plane.c  |  3 ++-
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c  | 16 +---
 drivers/gpu/drm/ingenic/ingenic-ipu.c  |  8 +---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c |  4 +++-
 drivers/gpu/drm/tilcdc/tilcdc_plane.c  |  3 ++-
 5 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c 
b/drivers/gpu/drm/imx/ipuv3-plane.c
index b5f6123850bb..6484592e3f86 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -341,7 +341,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 {
struct drm_plane_state *new_state = 
drm_atomic_get_new_plane_state(state,
   
plane);
-   struct drm_plane_state *old_state = plane->state;
+   struct drm_plane_state *old_state = 
drm_atomic_get_old_plane_state(state,
+  
plane);
struct drm_crtc_state *crtc_state;
struct device *dev = plane->dev->dev;
struct drm_framebuffer *fb = new_state->fb;
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index e6d7d0a04ddb..c022d9f1e737 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -361,11 +361,13 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc 
*crtc,
 static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
  struct drm_atomic_state *state)
 {
+   struct drm_plane_state *old_plane_state = 
drm_atomic_get_old_plane_state(state,
+   
 plane);
struct drm_plane_state *new_plane_state = 
drm_atomic_get_new_plane_state(state,

 plane);
struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
struct drm_crtc_state *crtc_state;
-   struct drm_crtc *crtc = new_plane_state->crtc ?: plane->state->crtc;
+   struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
int ret;
 
if (!crtc)
@@ -399,12 +401,12 @@ static int ingenic_drm_plane_atomic_check(struct 
drm_plane *plane,
 * its position, size or depth.
 */
if (priv->soc_info->has_osd &&
-   (!plane->state->fb || !new_plane_state->fb ||
-plane->state->crtc_x != new_plane_state->crtc_x ||
-plane->state->crtc_y != new_plane_state->crtc_y ||
-plane->state->crtc_w != new_plane_state->crtc_w ||
-plane->state->crtc_h != new_plane_state->crtc_h ||
-plane->state->fb->format->format != 
new_plane_state->fb->format->format))
+   (!old_plane_state->fb || !new_plane_state->fb ||
+old_plane_state->crtc_x != new

[PATCH v3 03/11] drm/atmel-hlcdc: Rename custom plane state variable

[Maxime Ripard]

Subsequent reworks will pass the global atomic state in the function
prototype, and atomic_check and atomic_update already have such a
variable already. Let's change them to ease the rework.

Acked-by: Sam Ravnborg 
Signed-off-by: Maxime Ripard 
---
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c   | 118 +-
 1 file changed, 59 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 15bc93163833..c62e930bccad 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -596,16 +596,16 @@ static int atmel_hlcdc_plane_atomic_check(struct 
drm_plane *p,
  struct drm_plane_state *s)
 {
struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
-   struct atmel_hlcdc_plane_state *state =
+   struct atmel_hlcdc_plane_state *hstate =
drm_plane_state_to_atmel_hlcdc_plane_state(s);
const struct atmel_hlcdc_layer_desc *desc = plane->layer.desc;
-   struct drm_framebuffer *fb = state->base.fb;
+   struct drm_framebuffer *fb = hstate->base.fb;
const struct drm_display_mode *mode;
struct drm_crtc_state *crtc_state;
int ret;
int i;
 
-   if (!state->base.crtc || WARN_ON(!fb))
+   if (!hstate->base.crtc || WARN_ON(!fb))
return 0;
 
crtc_state = drm_atomic_get_existing_crtc_state(s->state, s->crtc);
@@ -617,94 +617,94 @@ static int atmel_hlcdc_plane_atomic_check(struct 
drm_plane *p,
if (ret || !s->visible)
return ret;
 
-   state->src_x = s->src.x1;
-   state->src_y = s->src.y1;
-   state->src_w = drm_rect_width(>src);
-   state->src_h = drm_rect_height(>src);
-   state->crtc_x = s->dst.x1;
-   state->crtc_y = s->dst.y1;
-   state->crtc_w = drm_rect_width(>dst);
-   state->crtc_h = drm_rect_height(>dst);
+   hstate->src_x = s->src.x1;
+   hstate->src_y = s->src.y1;
+   hstate->src_w = drm_rect_width(>src);
+   hstate->src_h = drm_rect_height(>src);
+   hstate->crtc_x = s->dst.x1;
+   hstate->crtc_y = s->dst.y1;
+   hstate->crtc_w = drm_rect_width(>dst);
+   hstate->crtc_h = drm_rect_height(>dst);
 
-   if ((state->src_x | state->src_y | state->src_w | state->src_h) &
+   if ((hstate->src_x | hstate->src_y | hstate->src_w | hstate->src_h) &
SUBPIXEL_MASK)
return -EINVAL;
 
-   state->src_x >>= 16;
-   state->src_y >>= 16;
-   state->src_w >>= 16;
-   state->src_h >>= 16;
+   hstate->src_x >>= 16;
+   hstate->src_y >>= 16;
+   hstate->src_w >>= 16;
+   hstate->src_h >>= 16;
 
-   state->nplanes = fb->format->num_planes;
-   if (state->nplanes > ATMEL_HLCDC_LAYER_MAX_PLANES)
+   hstate->nplanes = fb->format->num_planes;
+   if (hstate->nplanes > ATMEL_HLCDC_LAYER_MAX_PLANES)
return -EINVAL;
 
-   for (i = 0; i < state->nplanes; i++) {
+   for (i = 0; i < hstate->nplanes; i++) {
unsigned int offset = 0;
int xdiv = i ? fb->format->hsub : 1;
int ydiv = i ? fb->format->vsub : 1;
 
-   state->bpp[i] = fb->format->cpp[i];
-   if (!state->bpp[i])
+   hstate->bpp[i] = fb->format->cpp[i];
+   if (!hstate->bpp[i])
return -EINVAL;
 
-   switch (state->base.rotation & DRM_MODE_ROTATE_MASK) {
+   switch (hstate->base.rotation & DRM_MODE_ROTATE_MASK) {
case DRM_MODE_ROTATE_90:
-   offset = (state->src_y / ydiv) *
+   offset = (hstate->src_y / ydiv) *
 fb->pitches[i];
-   offset += ((state->src_x + state->src_w - 1) /
-  xdiv) * state->bpp[i];
-   state->xstride[i] = -(((state->src_h - 1) / ydiv) *
+   offset += ((hstate->src_x + hstate->src_w - 1) /
+  xdiv) * hstate->bpp[i];
+   hstate->xstride[i] = -(((hstate->src_h - 1) / ydiv) *
fb->pitches[i]) -
- (2 * state->bpp[i]);
-   state->pstride[i] = fb->pitches[i] - state->bpp[i];
+ (2 * hstate->bpp[i]);
+   hstate->pstride[i]

Re: [PATCH RESEND v3 2/2] ARM: dts: sun4i: Add support for Topwise A721 tablet

[Maxime Ripard]

Hi,

On Tue, Feb 16, 2021 at 05:59:54PM +0100, Pascal Roeleven wrote:
> The Topwise A721/LY-F1 tablet is a tablet sold around 2012 under
> different brands. The mainboard mentions A721 clearly, so this tablet
> is best known under this name.
> 
> Signed-off-by: Pascal Roeleven 
> ---
>  arch/arm/boot/dts/Makefile   |   3 +-
>  arch/arm/boot/dts/sun4i-a10-topwise-a721.dts | 242 +++
>  2 files changed, 244 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/sun4i-a10-topwise-a721.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 3d1ea0b251..ba25b4235a 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1103,7 +1103,8 @@ dtb-$(CONFIG_MACH_SUN4I) += \
>   sun4i-a10-olinuxino-lime.dtb \
>   sun4i-a10-pcduino.dtb \
>   sun4i-a10-pcduino2.dtb \
> - sun4i-a10-pov-protab2-ips9.dtb
> + sun4i-a10-pov-protab2-ips9.dtb \
> + sun4i-a10-topwise-a721.dtb
>  dtb-$(CONFIG_MACH_SUN5I) += \
>   sun5i-a10s-auxtek-t003.dtb \
>   sun5i-a10s-auxtek-t004.dtb \
> diff --git a/arch/arm/boot/dts/sun4i-a10-topwise-a721.dts 
> b/arch/arm/boot/dts/sun4i-a10-topwise-a721.dts
> new file mode 100644
> index 00..936171d30b
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun4i-a10-topwise-a721.dts
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2020 Pascal Roeleven 
> + */
> +
> +/dts-v1/;
> +#include "sun4i-a10.dtsi"
> +#include "sunxi-common-regulators.dtsi"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/ {
> + model = "Topwise A721";
> + compatible = "topwise,a721", "allwinner,sun4i-a10";
> +
> + aliases {
> + serial0 = 
> + };
> +
> + backlight: backlight {
> + compatible = "pwm-backlight";
> + pwms = < 0 10 PWM_POLARITY_INVERTED>;
> + power-supply = <_vbat>;
> + enable-gpios = < 7 7 GPIO_ACTIVE_HIGH>; /* PH7 */
> + brightness-levels = <0 30 40 50 60 70 80 90 100>;
> + default-brightness-level = <8>;
> + };
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + panel: panel {
> + compatible = "starry,kr070pe2t";
> + backlight = <>;
> + power-supply = <_lcd_power>;
> +
> + port {
> + panel_input: endpoint {
> + remote-endpoint = <_out_panel>;
> + };
> + };
> + };
> +
> + reg_lcd_power: reg-lcd-power {
> + compatible = "regulator-fixed";
> + regulator-name = "reg-lcd-power";
> + gpio = < 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
> + enable-active-high;
> + };
> +
> + reg_vbat: reg-vbat {
> + compatible = "regulator-fixed";
> + regulator-name = "vbat";
> + regulator-min-microvolt = <370>;
> + regulator-max-microvolt = <370>;
> + };
> +
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + cpu-supply = <_dcdc2>;
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +
> + axp209: pmic@34 {
> + reg = <0x34>;
> + interrupts = <0>;
> + };
> +};
> +
> +#include "axp209.dtsi"
> +
> +_power_supply {
> + status = "okay";
> +};
> +
> +_power_supply {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +
> + mma7660: accelerometer@4c {

Chances are you don't need that label?

> + compatible = "fsl,mma7660";
> + reg = <0x4c>;
> + };
> +};
> +
> + {
> + status = "okay";
> +
> + ft5406ee8: touchscreen@38 {

Ditto

> + compatible = "edt,edt-ft5406";
> + reg = <0x38>;
> + interrupt-parent = <>;
> + interrupts = <7 21 IRQ_TYPE_EDGE_FALLING>;
> + touchscreen-size-x = <800>;
> + touchscreen-size-y = <480>;
> + vcc-supply = <_vcc3v3>;
> + };
> +};
> +
> + {
> + vref-supply = <_ldo2>;
> + status = "okay";
> +
> + button-vol-down {
> + label = "Volume Down";
> + linux,code = ;
> + channel = <0>;
> + voltage = <761904>;
> + };
> +
> + button-vol-up {
> + label = "Volume Up";
> + linux,code = ;
> + channel = <0>;
> + voltage = <571428>;
> + };

Those two nodes will raise a warning during the DT validation, please
make sure it doesn't have any warning.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] clk: sunxi-ng: v3s: add support for variable rate audio pll output

[Maxime Ripard]

On Thu, Feb 18, 2021 at 05:21:16PM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2021年2月18日 GMT+08:00 下午5:18:39, Tobias Schramm  写到:
> >Hi Icenowy,
> >
> > > We have introducee SDM-based accurate audio PLL on several
> >> other SoCs. Some people is quite sensitive about audio-related
> >things.
> > >
> >While it is possible to support 24MHz * 128 / 25 / 5 = 24.576MHz
> >without 
> >delta sigma modulation, matching 22.5792MHz is indeed not possible. I 
> >read you'd prefer me to use SDM like the other SoCs though? Shall I
> >send 
> >a v2 utilizing SDM?
> 
> Yes, I think so.

Yes I'd rather have consistency about how we deal with this across all SoCs

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] clk: sunxi-ng: v3s: add support for variable rate audio pll output

[Maxime Ripard]

Hi,

On Fri, Feb 12, 2021 at 02:57:25PM +0100, Tobias Schramm wrote:
> Previously the variable rate audio pll output was fixed to a divider of
> four. This is unfortunately incompatible with generating commonly used
> I2S core clock rates like 24.576MHz from the 24MHz parent clock.
> This commit adds support for arbitrary audio pll output dividers to fix
> that.
> 
> Signed-off-by: Tobias Schramm 

It's not really clear to me how that would help.

The closest frequency we can provide for 24.576MHz would be 24580645 Hz,
with N = 127, M = 31 and P = 4, so it would work with what we have
already?

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] pinctrl/sunxi: adding input-debounce-ns property

[Maxime Ripard]

Hi,

On Wed, Feb 10, 2021 at 05:22:37PM +0100, Marjan Pascolo wrote:
> On Allwinner SoC interrupt debounce can be controlled by two oscillator
> (32KHz and 24MHz) and a prescale divider.
> Oscillator and prescale divider are set through
> device tree property "input-debounce" which have 1uS accuracy.
> For acheive nS precision a new device tree poperty is made
> named "input-debounce-ns".
> "input-debounce-ns" is checked only if "input-debounce"
> property is not defined.
> 
> Suggested-by: Maxime Ripard 
> Signed-off-by: Marjan Pascolo 
> ---
> ---
>  .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  |  9 +++
>  drivers/pinctrl/sunxi/pinctrl-sunxi.c | 25 ---
>  2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> index 5240487dfe50..346776de3a44 100644
> ---
> a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> +++
> b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> @@ -93,6 +93,15 @@ properties:
>  minItems: 1
>  maxItems: 5
> 
> +  input-debounce-ns:
> +    description:
> +  Debouncing periods in nanoseconds, one period per interrupt
> +  bank found in the controller.
> +  Only checked if input-debounce is not present
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 5
> +

This should be a separate patch, with the DT maintainers in Cc. 

You should enforce that the properties are mutually exclusive through
the schema too

>  patternProperties:
>    # It's pretty scary, but the basic idea is that:
>    #   - One node name can start with either s- or r- for PRCM nodes,
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> index dc8d39ae045b..869b6d5743ba 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
> @@ -1335,14 +1335,31 @@ static int sunxi_pinctrl_setup_debounce(struct
> sunxi_pinctrl *pctl,
>  struct clk *hosc, *losc;
>  u8 div, src;
>  int i, ret;
> +    /* Keeping for loop below clean */
> +    const char* debounce_prop_name;
> +    unsigned long debounce_dividend;
> 
>  /* Deal with old DTs that didn't have the oscillators */
>  if (of_clk_get_parent_count(node) != 3)
>      return 0;
> 
> +    /*
> +     * Distinguish between simple input-debounce
> +     * and new input-debounce-ns
> +     */
> +

I'm not sure that comment should stay, the code is obvious enough

>  /* If we don't have any setup, bail out */
> -    if (!of_find_property(node, "input-debounce", NULL))
> -        return 0;
> +    if (!of_find_property(node, "input-debounce", NULL)) {
> +        if(!of_find_property(node, "input-debounce-ns", NULL)) {
> +            return 0;
> +        } else {
> +            debounce_prop_name="input-debounce-ns";
> +            debounce_dividend=NSEC_PER_SEC;
> +        }
> +    } else {
> +        debounce_prop_name="input-debounce";
> +        debounce_dividend=USEC_PER_SEC;
> +    }

This doesn't follow the kernel coding style, make sure to run
scripts/checkpatch.pl on your patches before sending them.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] Revert "ARM: dts: bcm2711: Add the BSC interrupt controller"

[Maxime Ripard]

On Fri, Feb 12, 2021 at 11:11:04AM -0800, Florian Fainelli wrote:
> As Dave reported:
> 
> This seems to have unintended side effects.  GIC interrupt 117 is shared
> between the standard I2C controllers (i2c-bcm2835) and the l2-intc block
> handling the HDMI I2C interrupts.
> 
> There is not a great way to share an interrupt between an interrupt
> controller using the chained IRQ handler which is an interrupt flow and
> another driver like i2c-bcm2835 which uses an interrupt handler
> (although it specifies IRQF_SHARED).
> 
> Simply revert this change for now which will mean that HDMI I2C will be
> polled, like it was before.
> 
> Reported-by: Dave Stevenson 
> Signed-off-by: Florian Fainelli 

Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller

[Maxime Ripard]

On Fri, Feb 12, 2021 at 12:20:14PM +0100, Nicolas Saenz Julienne wrote:
> On Wed, 2021-02-10 at 10:49 -0800, Florian Fainelli wrote:
> > On 2/10/21 7:49 AM, Dave Stevenson wrote:
> > > Hi Marc.
> > > 
> > > On Wed, 10 Feb 2021 at 15:30, Marc Zyngier  wrote:
> > > > 
> > > > Hi Maxime,
> > > > 
> > > > On 2021-02-10 14:40, Maxime Ripard wrote:
> > > > > Hi Dave,
> > > > > 
> > > > > On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote:
> > > > > > On Mon, 11 Jan 2021 at 14:23, Maxime Ripard  
> > > > > > wrote:
> > > > > > > 
> > > > > > > The BSC controllers used for the HDMI DDC have an interrupt 
> > > > > > > controller
> > > > > > > shared between both instances. Let's add it to avoid polling.
> > > > > > 
> > > > > > This seems to have unintended side effects.
> > > > > > GIC interrupt 117 is shared between the standard I2C controllers
> > > > > > (i2c-bcm2835) and the l2-intc block handling the HDMI I2C 
> > > > > > interrupts.
> > > > > > 
> > > > > > Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that
> > > > > > doesn't appear to be an option for l2-intc registering as an 
> > > > > > interrupt
> > > > > > controller. i2c-bcm2835 therefore loses out and fails to register 
> > > > > > for
> > > > > > the interrupt.
> > > > > > 
> > > > > > Is there an equivalent flag that an interrupt controller can add to
> > > > > > say that the parent interrupt is shared? Is that even supported?
> > > > > 
> > > > > Indeed, it looks like setting an equivalent to IRQF_SHARED would be 
> > > > > the
> > > > > solution, but I couldn't find anything that would allow us to in the
> > > > > irqchip code.
> > > > > 
> > > > > Marc, Thomas, is it something that is allowed?
> > > > 
> > > > No, not really. That's because the chained handler is actually an
> > > > interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong
> > > > level for that.
> > > > 
> > > > I can see two possibilities:
> > > > 
> > > > - the l2-intc gets turned into a normal handler, and does the demux
> > > >    from there. Horrible stuff.
> > > > 
> > > > - the i2c controller gets parented to the l2c-int as a fake interrupt,
> > > >    and gets called from there. Horrible stuff.
> > > > 
> > > > Pick your poison... :-/
> > > 
> > > Thanks for the info.
> > > 
> > > Option 3 - remove l2-intc and drop back to polling the i2c-brcmstb
> > > blocks (which the driver supports anyway).
> > > HDMI I2C generally isn't heavily used once displays are connected, so
> > > I'd be OK with that.
> > > 
> > > (We can keep the l2-intc that handles CEC and HPD as that is on a
> > > unique GIC interrupt).
> > 
> > Agreed, Maxime or Nicolas do you want me to send a revert of this patch?
> 
> Reverting seems the safe move, but I'll defer to whatever Maxime says.

Yes, reverting it seems like the easiest way forward. If you can send it
Florian that would be great :)

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2] arm: dts: sun5i: Add GPU node

[Maxime Ripard]

Hi,

On Thu, Feb 11, 2021 at 03:25:23PM +, Yassine Oudjana wrote:
> sun5i has the same Mali 400 GPU as sun4i with the same interrupts, clocks
> and resets. Add node for it in dts.
> 
> Signed-off-by: Yassine Oudjana 

Unfortunately we already merged a similar patch for 5.12

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] ARM: dts: sun8i: h3: orangepi-plus: Fix Ethernet PHY mode

[Maxime Ripard]

Hi,

On Mon, Feb 08, 2021 at 12:24:57PM +0100, B.R. Oake wrote:
> Since commit bbc4d71d6354 ("net: phy: realtek: fix rtl8211e rx/tx
> delay config"), Ethernet no longer works on the Orange Pi Plus,
> because that commit sets the RX/TX delay according to the phy-mode
> property in the device tree, which is "rgmii", the wrong setting
> for this board.
> 
> Following the example of others who fixed the same problem for
> many other boards, this patch changes the phy-mode to "rgmii-id"
> which gets Ethernet working again on this board.
> 
> Fixes: 4904337fe34f ("ARM: dts: sunxi: Restore EMAC changes (boards)")
> Fixes: 1dcd0095019a ("ARM: sun8i: orangepi-plus: Enable dwmac-sun8i")
> Signed-off-by: B.R. Oake 

Unfortunately we can't take this patch as is, this needs to be your real name, 
see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller

[Maxime Ripard]

Hi Dave,

On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote:
> On Mon, 11 Jan 2021 at 14:23, Maxime Ripard  wrote:
> >
> > The BSC controllers used for the HDMI DDC have an interrupt controller
> > shared between both instances. Let's add it to avoid polling.
> 
> This seems to have unintended side effects.
> GIC interrupt 117 is shared between the standard I2C controllers
> (i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts.
> 
> Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that
> doesn't appear to be an option for l2-intc registering as an interrupt
> controller. i2c-bcm2835 therefore loses out and fails to register for
> the interrupt.
> 
> Is there an equivalent flag that an interrupt controller can add to
> say that the parent interrupt is shared? Is that even supported?

Indeed, it looks like setting an equivalent to IRQF_SHARED would be the
solution, but I couldn't find anything that would allow us to in the
irqchip code.

Marc, Thomas, is it something that is allowed?

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 0/5] sunxi: fix H6 HDMI related issues

[Maxime Ripard]

On Tue, Feb 09, 2021 at 06:58:55PM +0100, Jernej Skrabec wrote:
> Over the year I got plenty of reports of troubles with H6 HDMI signal.
> Sometimes monitor flickers, sometimes there was no image at all and
> sometimes it didn't play well with AVR.
> 
> It turns out there are multiple issues. Patch 1 fixes clock issue,
> which didn't adjust parent rate, even if it is allowed to do so. Patch 2
> adds polarity config in tcon1. This is seemingly not needed for pre-HDMI2
> controllers, although BSP drivers set it accordingly every time. It
> turns out that HDMI2 controllers often don't work with monitors if
> polarity is not set correctly. Patch 3 always set clock rate for HDMI
> controller. Patch 4 fixes H6 HDMI PHY settings. Patch 5 fixes comment and
> clock rate limit (wrong reasoning).
> 
> Please take a look.
> 
> Best regards,
> Jernej
> 
> Changes from v2:
> - use clk_hw_can_set_rate_parent() directly instead of checking flags
> Changes from v1:
> - collected Chen-Yu tags (except on replaced patch 4)
> - Added some comments in patch 2
> - Replaced patch 4 (see commit log for explanation)

Applied patches 2-5, thanks
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v3 1/5] clk: sunxi-ng: mp: fix parent rate change flag check

[Maxime Ripard]

Hi Mike, Stephen,

On Tue, Feb 09, 2021 at 06:58:56PM +0100, Jernej Skrabec wrote:
> CLK_SET_RATE_PARENT flag is checked on parent clock instead of current
> one. Fix that.
> 
> Fixes: 3f790433c3cb ("clk: sunxi-ng: Adjust MP clock parent rate when 
> allowed")
> Reviewed-by: Chen-Yu Tsai 
> Tested-by: Andre Heider 
> Signed-off-by: Jernej Skrabec 

This is a last minute fix for us, can you merge it into clk-fixes directly?

Acked-by: Maxime Ripard 

Thanks!
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v5 0/2] Implement DE2.0 and DE3.0 per-plane alpha support

[Maxime Ripard]

On Thu, Jan 28, 2021 at 01:39:38PM +0200, Roman Stratiienko wrote:
> 
> Please review/merge.
> 
> v2:
> Initial patch
> 
> v3:
> - Skip adding & applying alpha property if VI count > 1 (v3s case)
> 
> v4:
> Resend (author's email changed)
> 
> v5:
> Resend

Applied, thanks
Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Maxime Ripard]

On Sat, Feb 06, 2021 at 09:27:30PM +0800, 班涛 wrote:
> Maxime Ripard  于2021年2月6日周六 上午12:12写道：
> 
> > Hi,
> >
> > On Thu, Feb 04, 2021 at 11:47:34AM +0800, 班涛 wrote:
> > > Maxime Ripard  于2021年2月3日周三 下午11:46写道：
> > >
> > > > Hi,
> > > >
> > > > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > > > > From: Ban Tao 
> > > > >
> > > > > The Allwinner R818, A133, R329, V536 and V833 has a new PWM
> > controller
> > > > > IP compared to the older Allwinner SoCs.
> > > > >
> > > > > Signed-off-by: Ban Tao 
> > > >
> > > > Thanks for your patch. There's a bunch of warnings reported by
> > > > checkpatch --strict, they should be addressed.
> > > >
> > > > > ---
> > > > > v1->v2:
> > > > > 1.delete unnecessary code.
> > > > > 2.using a named define for some constants.
> > > > > 3.Add comment in sun50i_pwm_config function.
> > > > > 4.using dev_err_probe() for error handling.
> > > > > 5.disable the clock after pwmchip_remove().
> > > > > ---
> > > > >  MAINTAINERS  |   6 +
> > > > >  drivers/pwm/Kconfig  |  11 ++
> > > > >  drivers/pwm/Makefile |   1 +
> > > > >  drivers/pwm/pwm-sun50i.c | 348
> > +++
> > > > >  4 files changed, 366 insertions(+)
> > > > >  create mode 100644 drivers/pwm/pwm-sun50i.c
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index e73636b75f29..d33cf1b69b43 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -737,6 +737,12 @@ L:   linux-me...@vger.kernel.org
> > > > >  S:   Maintained
> > > > >  F:   drivers/staging/media/sunxi/cedrus/
> > > > >
> > > > > +ALLWINNER PWM DRIVER
> > > > > +M:   Ban Tao 
> > > > > +L:   linux-...@vger.kernel.org
> > > > > +S:   Maintained
> > > > > +F:   drivers/pwm/pwm-sun50i.c
> > > > > +
> > > > >  ALPHA PORT
> > > > >  M:   Richard Henderson 
> > > > >  M:   Ivan Kokshaysky 
> > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > index 9a4f66ae8070..17635a8f2ed3 100644
> > > > > --- a/drivers/pwm/Kconfig
> > > > > +++ b/drivers/pwm/Kconfig
> > > > > @@ -552,6 +552,17 @@ config PWM_SUN4I
> > > > > To compile this driver as a module, choose M here: the module
> > > > > will be called pwm-sun4i.
> > > > >
> > > > > +config PWM_SUN50I
> > > > > + tristate "Allwinner enhanced PWM support"
> > > > > + depends on ARCH_SUNXI || COMPILE_TEST
> > > > > + depends on HAS_IOMEM && COMMON_CLK
> > > > > + help
> > > > > +   Enhanced PWM framework driver for Allwinner R818, A133, R329,
> > > > > +   V536 and V833 SoCs.
> > > > > +
> > > > > +   To compile this driver as a module, choose M here: the module
> > > > > +   will be called pwm-sun50i.
> > > > > +
> > > >
> > > > Even though it's unfortunate, there's a bunch of other SoCs part of the
> > > > sun50i family that are supported by the sun4i driver.
> > > >
> > > > Which SoC introduced that new design? It's usually the name we pick up
> > > > then.
> > > >
> > >
> > > In fact, some SoCs has not been supported by the sun4i driver, such as
> > v833,
> > > v536, r818, a133 and r329.
> > > v833 and v536 are part of the sun8i family, but r818, a133 and r329 are
> > > part of the sun50i family.
> >
> > Indeed, I missed that sorry.
> >
> > > So,I'm confused, how do choose the name of this driver?
> >
> > Just go for the earliest SoC that introduced that design. What was the
> > first SoC to use it?
> >
>
> The V536 SOC first used this design, so, we should use the name
> "sun8i-pwm.c"?

sun8i-pwm would be too generic, but sun8i-v536-pwm would be great then

> 
> > > > > +static const struct sun50i_pwm_data sun8i_pwm_data_c9 = {
> > > > > + .npwm = 9,
> > > > > +};
> > > > > +
&

Re: [PATCH v5 1/2] dt-bindings: hwlock: add sun6i_hwspinlock

[Maxime Ripard]

On Sun, Feb 07, 2021 at 10:07:31AM +0100, Wilken Gottwalt wrote:
> On Wed, 23 Dec 2020 15:49:19 -0700
> Rob Herring  wrote:
> 
> > On Wed, Dec 23, 2020 at 4:34 AM Wilken Gottwalt
> >  wrote:
> > >
> > > Adds documentation on how to use the sun6i_hwspinlock driver for sun6i
> > > compatible SoCs.
> > 
> > Please resend to DT list so that automated checks run and it's in my
> > queue (PW). You need to run 'make dt_binding_check' as there are
> > several issues.
> 
> Mentioning somewhere, that yamllint is required would have helped here a lot.
> Without it I always ended up with that, what was quite misleading:
> 
> ERROR: dtschema minimum version is v2020.8.1
> make[1]: *** [Documentation/devicetree/bindings/Makefile:12: 
> check_dtschema_version] Error 1
> make: *** [Makefile:1370: dt_binding_check] Error 2
> 
> > > Signed-off-by: Wilken Gottwalt 
> > > ---
> > > Changes in v5:
> > >   - changed binding to earliest known supported SoC sun6i-a31
> > >   - dropped unnecessary entries
> > >
> > > Changes in v4:
> > >   - changed binding to sun8i-a33-hwpinlock
> > >   - added changes suggested by Maxime Ripard
> > >
> > > Changes in v3:
> > >   - changed symbols from sunxi to sun8i
> > >
> > > Changes in v2:
> > >   - fixed memory ranges
> > > ---
> > >  .../bindings/hwlock/sun6i-a31-hwspinlock.yaml | 44 +++
> > >  1 file changed, 44 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > >
> > > diff --git 
> > > a/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > > b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml new 
> > > file mode 100644
> > > index ..481c5c995ad7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwlock/sun6i-a31-hwspinlock.yaml
> > > @@ -0,0 +1,44 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwlock/sun6i-hwspinlock.yaml#
> > 
> > This will fail checks. Wrong filename.
> > 
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: SUN6I hardware spinlock driver for Allwinner sun6i compatible SoCs
> > > +
> > > +maintainers:
> > > +  - Wilken Gottwalt 
> > > +
> > > +description:
> > > +  The hardware unit provides semaphores between the ARM cores and the 
> > > embedded
> > > +  companion core on the SoC.
> > > +
> > > +properties:
> > > +  compatible:
> > > +const: allwinner,sun6i-a31-hwspinlock
> > > +
> > > +reg:
> > > +  maxItems: 1
> > > +
> > > +clocks:
> > > +  maxItems: 1
> > > +
> > > +resets:
> > > +  maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - resets
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +hwspinlock@1c18000 {
> > 
> > hwlock@...
> 
> sprd, stm32 and omap using hwspinlock. Why is it okay there and not okay in
> my version?

The spec mandates hwlock:

https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter2-devicetree-basics.rst#generic-names-recommendation

They probably introduced their nodes before it was standardized (or
didn't care).

> > > +  compatible = "allwinner,sun6i-a31-hwspinlock";
> > > +  reg = <0x01c18000 0x1000>;
> > > +  clocks = < CLK_BUS_SPINLOCK>;
> > > +  resets = < RST_BUS_SPINLOCK>;
> > 
> > You need an include for these defines.
> 
> So I guess it is needed because I the clocks/resets are used, right? But why
> is it not the case for the sprd example, which also uses clocks?

If you're talking about sprd,hwspinlock-r3p0, it doesn't use any define
(and thus doesn't need any include), and it's a binding in a text format
that wouldn't compile the example (this is only done for yaml bindings).
Even if it was wrong for them to do so, they wouldn't notice.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 2/5] drm/sun4i: tcon: set sync polarity for tcon1 channel

[Maxime Ripard]

On Fri, Feb 05, 2021 at 07:47:17PM +0100, Jernej Škrabec wrote:
> Dne petek, 05. februar 2021 ob 17:28:23 CET je Chen-Yu Tsai napisal(a):
> > On Sat, Feb 6, 2021 at 12:21 AM Jernej Škrabec  
> wrote:
> > >
> > > Dne petek, 05. februar 2021 ob 17:01:30 CET je Maxime Ripard napisal(a):
> > > > On Fri, Feb 05, 2021 at 11:21:22AM +0800, Chen-Yu Tsai wrote:
> > > > > On Fri, Feb 5, 2021 at 2:48 AM Jernej Skrabec 
> 
> > > wrote:
> > > > > >
> > > > > > Channel 1 has polarity bits for vsync and hsync signals but driver 
> never
> > > > > > sets them. It turns out that with pre-HDMI2 controllers seemingly 
> there
> > > > > > is no issue if polarity is not set. However, with HDMI2 controllers
> > > > > > (H6) there often comes to de-synchronization due to phase shift. 
> This
> > > > > > causes flickering screen. It's safe to assume that similar issues 
> might
> > > > > > happen also with pre-HDMI2 controllers.
> > > > > >
> > > > > > Solve issue with setting vsync and hsync polarity. Note that display
> > > > > > stacks with tcon top have polarity bits actually in tcon0 polarity
> > > > > > register.
> > > > > >
> > > > > > Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine 
> support")
> > > > > > Tested-by: Andre Heider 
> > > > > > Signed-off-by: Jernej Skrabec 
> > > > > > ---
> > > > > >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 24 
> > > > > >  drivers/gpu/drm/sun4i/sun4i_tcon.h |  5 +
> > > > > >  2 files changed, 29 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/
> sun4i/
> > > sun4i_tcon.c
> > > > > > index 6b9af4c08cd6..0d132dae58c0 100644
> > > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > > > > > @@ -672,6 +672,29 @@ static void sun4i_tcon1_mode_set(struct 
> sun4i_tcon
> > > *tcon,
> > > > > >  SUN4I_TCON1_BASIC5_V_SYNC(vsync) |
> > > > > >  SUN4I_TCON1_BASIC5_H_SYNC(hsync));
> > > > > >
> > > > > > +   /* Setup the polarity of sync signals */
> > > > > > +   if (tcon->quirks->polarity_in_ch0) {
> > > > > > +   val = 0;
> > > > > > +
> > > > > > +   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > > > > > +   val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> > > > > > +
> > > > > > +   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > > > +   val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > > > > > +
> > > > > > +   regmap_write(tcon->regs, SUN4I_TCON0_IO_POL_REG, 
> val);
> > > > > > +   } else {
> > > > > > +   val = SUN4I_TCON1_IO_POL_UNKNOWN;
> > > > >
> > > > > I think a comment for the origin of this is warranted.
> > > >
> > > > If it's anything like TCON0, it's the pixel clock polarity
> > >
> > > Hard to say, DW HDMI controller has "data enable" polarity along hsync and
> > > vsync. It could be either or none of those.
> > >
> > > What should I write in comment? BSP drivers and documentation use only 
> generic
> > > names like io2_inv.
> > 
> > Just say that we don't know exactly what it is, but it is required for 
> things
> > to work properly? Would be interesting to know what happens if you don't set
> > this bit, but do set VSYNC/HSYNC polarity properly.
> 
> Nothing seems to happen - tested on H3 with HDMI (4k@30) and CVBS. At least I 
> didn't notice anything.

That's pretty normal, an inverted pixel clock would at worst give you
some weird artifacts and / or pixels being of the wrong color. Data
enable on the other hand would very likely stall the HDMI controller
since you would have only the blanking periods that would be considered
valid.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 2/5] drm/sun4i: tcon: set sync polarity for tcon1 channel

[Maxime Ripard]

On Fri, Feb 05, 2021 at 11:21:22AM +0800, Chen-Yu Tsai wrote:
> On Fri, Feb 5, 2021 at 2:48 AM Jernej Skrabec  wrote:
> >
> > Channel 1 has polarity bits for vsync and hsync signals but driver never
> > sets them. It turns out that with pre-HDMI2 controllers seemingly there
> > is no issue if polarity is not set. However, with HDMI2 controllers
> > (H6) there often comes to de-synchronization due to phase shift. This
> > causes flickering screen. It's safe to assume that similar issues might
> > happen also with pre-HDMI2 controllers.
> >
> > Solve issue with setting vsync and hsync polarity. Note that display
> > stacks with tcon top have polarity bits actually in tcon0 polarity
> > register.
> >
> > Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
> > Tested-by: Andre Heider 
> > Signed-off-by: Jernej Skrabec 
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 24 
> >  drivers/gpu/drm/sun4i/sun4i_tcon.h |  5 +
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c 
> > b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 6b9af4c08cd6..0d132dae58c0 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -672,6 +672,29 @@ static void sun4i_tcon1_mode_set(struct sun4i_tcon 
> > *tcon,
> >  SUN4I_TCON1_BASIC5_V_SYNC(vsync) |
> >  SUN4I_TCON1_BASIC5_H_SYNC(hsync));
> >
> > +   /* Setup the polarity of sync signals */
> > +   if (tcon->quirks->polarity_in_ch0) {
> > +   val = 0;
> > +
> > +   if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > +   val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> > +
> > +   if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > +   val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > +
> > +   regmap_write(tcon->regs, SUN4I_TCON0_IO_POL_REG, val);
> > +   } else {
> > +   val = SUN4I_TCON1_IO_POL_UNKNOWN;
> 
> I think a comment for the origin of this is warranted.

If it's anything like TCON0, it's the pixel clock polarity

Maxime

Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Maxime Ripard]

Hi,

On Thu, Feb 04, 2021 at 11:47:34AM +0800, 班涛 wrote:
> Maxime Ripard  于2021年2月3日周三 下午11:46写道：
> 
> > Hi,
> >
> > On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> > > From: Ban Tao 
> > >
> > > The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> > > IP compared to the older Allwinner SoCs.
> > >
> > > Signed-off-by: Ban Tao 
> >
> > Thanks for your patch. There's a bunch of warnings reported by
> > checkpatch --strict, they should be addressed.
> >
> > > ---
> > > v1->v2:
> > > 1.delete unnecessary code.
> > > 2.using a named define for some constants.
> > > 3.Add comment in sun50i_pwm_config function.
> > > 4.using dev_err_probe() for error handling.
> > > 5.disable the clock after pwmchip_remove().
> > > ---
> > >  MAINTAINERS  |   6 +
> > >  drivers/pwm/Kconfig  |  11 ++
> > >  drivers/pwm/Makefile |   1 +
> > >  drivers/pwm/pwm-sun50i.c | 348 +++
> > >  4 files changed, 366 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sun50i.c
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e73636b75f29..d33cf1b69b43 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -737,6 +737,12 @@ L:   linux-me...@vger.kernel.org
> > >  S:   Maintained
> > >  F:   drivers/staging/media/sunxi/cedrus/
> > >
> > > +ALLWINNER PWM DRIVER
> > > +M:   Ban Tao 
> > > +L:   linux-...@vger.kernel.org
> > > +S:   Maintained
> > > +F:   drivers/pwm/pwm-sun50i.c
> > > +
> > >  ALPHA PORT
> > >  M:   Richard Henderson 
> > >  M:   Ivan Kokshaysky 
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 9a4f66ae8070..17635a8f2ed3 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -552,6 +552,17 @@ config PWM_SUN4I
> > > To compile this driver as a module, choose M here: the module
> > > will be called pwm-sun4i.
> > >
> > > +config PWM_SUN50I
> > > + tristate "Allwinner enhanced PWM support"
> > > + depends on ARCH_SUNXI || COMPILE_TEST
> > > + depends on HAS_IOMEM && COMMON_CLK
> > > + help
> > > +   Enhanced PWM framework driver for Allwinner R818, A133, R329,
> > > +   V536 and V833 SoCs.
> > > +
> > > +   To compile this driver as a module, choose M here: the module
> > > +   will be called pwm-sun50i.
> > > +
> >
> > Even though it's unfortunate, there's a bunch of other SoCs part of the
> > sun50i family that are supported by the sun4i driver.
> >
> > Which SoC introduced that new design? It's usually the name we pick up
> > then.
> >
> 
> In fact, some SoCs has not been supported by the sun4i driver, such as v833,
> v536, r818, a133 and r329.
> v833 and v536 are part of the sun8i family, but r818, a133 and r329 are
> part of the sun50i family.

Indeed, I missed that sorry. 

> So,I'm confused, how do choose the name of this driver?

Just go for the earliest SoC that introduced that design. What was the
first SoC to use it?

> > > +static const struct sun50i_pwm_data sun8i_pwm_data_c9 = {
> > > + .npwm = 9,
> > > +};
> > > +
> > > +static const struct sun50i_pwm_data sun50i_pwm_data_c16 = {
> > > + .npwm = 16,
> > > +};
> > > +
> > > +static const struct of_device_id sun50i_pwm_dt_ids[] = {
> > > + {
> > > + .compatible = "allwinner,sun8i-v833-pwm",
> > > + .data = _pwm_data_c9,
> > > + }, {
> > > + .compatible = "allwinner,sun8i-v536-pwm",
> > > + .data = _pwm_data_c9,
> > > + }, {
> > > + .compatible = "allwinner,sun50i-r818-pwm",
> > > + .data = _pwm_data_c16,
> > > + }, {
> > > + .compatible = "allwinner,sun50i-a133-pwm",
> > > + .data = _pwm_data_c16,
> > > + }, {
> > > + .compatible = "allwinner,sun50i-r329-pwm",
> > > + .data = _pwm_data_c9,
> > > + }, {
> > > + /* sentinel */
> > > + },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, sun50i_pwm_dt_ids);
> >
> > What are the differences between all these SoCs? If there's none between
> > the v833, v536 and R329, and between the r818 and the A133, you should
> > use the same compatible.
> 
> Compared with the sun4i driver, this patch is a completely different PWM IP
> controller.

Sure, I didn't mean to compare it to sun4i. What I meant was that as far
as these struct goes, the A133 and the R818 look to have the same PWM
controller. Just like the v833, v536 and R329.

If the PWM controllers are indeed identical across those SoCs, you can
just use two compatibles, one for the PWM with 9 channels (again, the
earliest SoC among the V833, v536 and r329), and one for the PWM with 16
channels.

None of those SoCs are supported at the moment in Linux though, so it
would make more sense to support them first before adding a new driver
for those SoCs, it's gonna be dead code otherwise.

Maxime

Re: [PATCH v2] pwm: sunxi: Add Allwinner SoC PWM controller driver

[Maxime Ripard]

Hi,

On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote:
> From: Ban Tao 
> 
> The Allwinner R818, A133, R329, V536 and V833 has a new PWM controller
> IP compared to the older Allwinner SoCs.
> 
> Signed-off-by: Ban Tao 

Thanks for your patch. There's a bunch of warnings reported by
checkpatch --strict, they should be addressed.

> ---
> v1->v2:
> 1.delete unnecessary code.
> 2.using a named define for some constants.
> 3.Add comment in sun50i_pwm_config function.
> 4.using dev_err_probe() for error handling.
> 5.disable the clock after pwmchip_remove().
> ---
>  MAINTAINERS  |   6 +
>  drivers/pwm/Kconfig  |  11 ++
>  drivers/pwm/Makefile |   1 +
>  drivers/pwm/pwm-sun50i.c | 348 +++
>  4 files changed, 366 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun50i.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e73636b75f29..d33cf1b69b43 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -737,6 +737,12 @@ L:   linux-me...@vger.kernel.org
>  S:   Maintained
>  F:   drivers/staging/media/sunxi/cedrus/
>  
> +ALLWINNER PWM DRIVER
> +M:   Ban Tao 
> +L:   linux-...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/pwm/pwm-sun50i.c
> +
>  ALPHA PORT
>  M:   Richard Henderson 
>  M:   Ivan Kokshaysky 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 9a4f66ae8070..17635a8f2ed3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -552,6 +552,17 @@ config PWM_SUN4I
> To compile this driver as a module, choose M here: the module
> will be called pwm-sun4i.
>  
> +config PWM_SUN50I
> + tristate "Allwinner enhanced PWM support"
> + depends on ARCH_SUNXI || COMPILE_TEST
> + depends on HAS_IOMEM && COMMON_CLK
> + help
> +   Enhanced PWM framework driver for Allwinner R818, A133, R329,
> +   V536 and V833 SoCs.
> +
> +   To compile this driver as a module, choose M here: the module
> +   will be called pwm-sun50i.
> +

Even though it's unfortunate, there's a bunch of other SoCs part of the
sun50i family that are supported by the sun4i driver.

Which SoC introduced that new design? It's usually the name we pick up
then.

>  config PWM_TEGRA
>   tristate "NVIDIA Tegra PWM support"
>   depends on ARCH_TEGRA || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 6374d3b1d6f3..b4754927fd8f 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)   += pwm-stm32-lp.o
>  obj-$(CONFIG_PWM_STMPE)  += pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)  += pwm-sun4i.o
> +obj-$(CONFIG_PWM_SUN50I) += pwm-sun50i.o
>  obj-$(CONFIG_PWM_TEGRA)  += pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)   += pwm-tiehrpwm.o
> diff --git a/drivers/pwm/pwm-sun50i.c b/drivers/pwm/pwm-sun50i.c
> new file mode 100644
> index ..37285d771924
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for Allwinner sun50i Pulse Width Modulation Controller
> + *
> + * Copyright (C) 2020 Ban Tao 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PWM_GET_CLK_OFFSET(chan) (0x20 + ((chan >> 1) * 0x4))
> +#define PWM_CLK_APB_SCR  BIT(7)
> +#define PWM_DIV_M0
> +#define PWM_DIV_M_WIDTH  0x4
> +
> +#define PWM_CLK_REG  0x40
> +#define PWM_CLK_GATING   BIT(0)
> +
> +#define PWM_ENABLE_REG   0x80
> +#define PWM_EN   BIT(0)
> +
> +#define PWM_CTL_REG(chan)(0x100 + 0x20 * chan)
> +#define PWM_ACT_STA  BIT(8)
> +#define PWM_PRESCAL_K0
> +#define PWM_PRESCAL_K_WIDTH  0x8
> +
> +#define PWM_PERIOD_REG(chan) (0x104 + 0x20 * chan)
> +#define PWM_ENTIRE_CYCLE 16
> +#define PWM_ENTIRE_CYCLE_WIDTH   0x10
> +#define PWM_ACT_CYCLE0
> +#define PWM_ACT_CYCLE_WIDTH  0x10
> +
> +#define BIT_CH(bit, chan)((bit) << (chan))
> +
> +#define SETMASK(width, shift)((width?((-1U) >> 
> (32-width)):0)  << (shift))
> +#define CLRMASK(width, shift)(~(SETMASK(width, shift)))
> +#define GET_BITS(shift, width, reg) \
> + (((reg) & SETMASK(width, shift)) >> (shift))
> +#define SET_BITS(shift, width, reg, val) \
> + (((reg) & CLRMASK(width, shift)) | (val << (shift)))
> +
> +#define PWM_OSC_CLK  2400
> +#define PWM_PRESCALER_MAX256
> +#define PWM_CLK_DIV_M__MAX   9
> +#define PWM_ENTIRE_CYCLE_MAX 65536
> +
> +struct sun50i_pwm_data {
> + unsigned int npwm;
> +};
> +
> +struct sun50i_pwm_chip {
> + 

Re: [PATCH 12/21] clk: sunxi: clk-sun6i-ar100: Demote non-conformant kernel-doc header

[Maxime Ripard]

On Tue, Jan 26, 2021 at 04:54:59PM +, Lee Jones wrote:
> On Tue, 26 Jan 2021, Maxime Ripard wrote:
> 
> > On Tue, Jan 26, 2021 at 12:45:31PM +, Lee Jones wrote:
> > > Fixes the following W=1 kernel build warning(s):
> > > 
> > >  drivers/clk/sunxi/clk-sun6i-ar100.c:26: warning: Function parameter or 
> > > member 'req' not described in 'sun6i_get_ar100_factors'
> > > 
> > > Cc: "Emilio López" 
> > > Cc: Michael Turquette 
> > > Cc: Stephen Boyd 
> > > Cc: Maxime Ripard 
> > > Cc: Chen-Yu Tsai 
> > > Cc: Jernej Skrabec 
> > > Cc: Boris BREZILLON 
> > > Cc: linux-...@vger.kernel.org
> > > Cc: linux-arm-ker...@lists.infradead.org
> > > Signed-off-by: Lee Jones 
> > > ---
> > >  drivers/clk/sunxi/clk-sun6i-ar100.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/sunxi/clk-sun6i-ar100.c 
> > > b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > index e1b7d0929cf7f..54babc2b4b9ee 100644
> > > --- a/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > +++ b/drivers/clk/sunxi/clk-sun6i-ar100.c
> > > @@ -16,7 +16,7 @@
> > >  
> > >  #include "clk-factors.h"
> > >  
> > > -/**
> > > +/*
> > >   * sun6i_get_ar100_factors - Calculates factors p, m for AR100
> > >   *
> > >   * AR100 rate is calculated as follows
> > 
> > This is the sixth patch doing the exact same thing over the files in
> > that folder you sent. Please fix all the occurences at once
> 
> No.  That would make the whole clean-up process 10x harder than it
> already is
> 
> Before starting this endeavour there were 18,000+ warnings spread over
> 100's of files and 10's of subsystems that needed addressing (only a
> couple thousand left now thankfully).  Some issues vastly different,
> some duplicated (much too much copy/pasting going which made things
> very frustrating at times).
> 
> Anyway, in order to work though them all gracefully and in a sensible
> time-frame I had to come up with a workable plan.  Each subsystem is
> compiled separately and a script attempts to take out duplicate
> warnings and takes me through the build-log one file at a time.  Once
> all of the warnings are fixed in a source-file, it moves on to the
> next file.  The method is clean and allows me to handle this
> gargantuan task in bite-sized chunks.

I mean, you have literally used the same commit log and the same changes
over six different files in the same directory. Sure changes across
different parts of the kernel can be painful, but it's really not what
we're discussing here.

> Going though and pairing up similar changes is unsustainable for a
> task like this.  It would add a lot of additional overhead and would
> slow down the rate of acceptance since source files tend to have
> different reviewers/maintainers - some working faster to review
> patches than others, leading to excessive lag times waiting for that
> one reviewer who takes weeks to review.

Are you arguing that sending the same patch 6 times is easier and faster
to review for the maintainer than the same changes in a single patch?

> Having each file addressed in a separate patch also helps
> revertability and bisectability. Not such a big problem with the
> documentation patches, but still.

There's nothing to revert or bisect, those changes aren't functional
changes.

> Admittedly doing it this way *can* look a bit odd in *some* patch-sets
> when they hit the MLs - particularly clock it seems, where there
> hasn't even been a vague attempt to document any of the parameters in
> the kernel-doc headers - however the alternative would mean nothing
> would get done!

Yeah, and even though properly documenting the functions would have been
the right way to fix those warnings, I didn't ask you to do that since I
was expecting it to be daunting. Surely we can meet half-way

Maxime


signature.asc
Description: PGP signature

Re: [PATCH v2] ARM: dts: sun5i: Add dts for inet86v_rev2

[Maxime Ripard]

On Mon, Feb 01, 2021 at 06:18:18PM +0100, agriveaux wrote:
> On Thu, Jan 28, 2021 at 06:23:29PM +0100, Maxime Ripard wrote:
> > Hi,
> Hi,
> > 
> > On Sun, Jan 24, 2021 at 08:39:03PM +0100, Alexandre GRIVEAUX wrote:
> > > Add Inet 86V Rev 2 support, based upon Inet 86VS.
> > > 
> > > The Inet 86V use SL1536 touchpanel controller, the Inet 86VS a GSL1680,
> > > which make them both incompatible.
> > > 
> > > Missing things:
> > > - Accelerometer (MXC6225X)
> > > - Touchpanel (Sitronix SL1536)
> > > - Nand (29F32G08CBACA)
> > > - Camera (HCWY0308)
> > > 
> > > Signed-off-by: Alexandre GRIVEAUX 
> > > ---
> > >  arch/arm/boot/dts/sun5i-a13-inet-86v-rev2.dts | 17 +
> > 
> > You have to add it to the Makefile
> > 
> Ok.
> > >  1 file changed, 17 insertions(+)
> > >  create mode 100644 arch/arm/boot/dts/sun5i-a13-inet-86v-rev2.dts
> > > 
> > > diff --git a/arch/arm/boot/dts/sun5i-a13-inet-86v-rev2.dts 
> > > b/arch/arm/boot/dts/sun5i-a13-inet-86v-rev2.dts
> > > new file mode 100644
> > > index ..581083e932d8
> > > --- /dev/null
> > > +++ b/arch/arm/boot/dts/sun5i-a13-inet-86v-rev2.dts
> > > @@ -0,0 +1,17 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2021 Alexandre Griveaux 
> > > + *
> > > + * Minimal dts file for the iNet 86V
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "sun5i-a13.dtsi"
> > > +#include "sun5i-reference-design-tablet.dtsi"
> > > +
> > > +/ {
> > > + model = "iNET 86V Rev 02";
> > > + compatible = "inet,86v-rev2", "allwinner,sun5i-a13";
> > 
> > inet should be documented in the vendor prefixes, and that compatible
> > should be documented in Documentation/devicetree/bindings/arm/sunxi.yaml
> > 
> 
> I forgot, but should be:
> 
>   - description: iNet-86V Rev 02
> items:
>   - const: primux,inet86v-rev2
>   - const: allwinner,sun5i-a13
> 
> > Having the first rev compatible would be good too
> 
> Unfortunatly, I didn't find inet86v rev1 on FCC website and on
> linux-sunxi. 
> 
> > 
> > > +
> > > +};
> > 
> > But I'm wondering. If there's nothing here to add, why would we need
> > that DT in the first place?
> > 
> I prefer to add often instead of bulk adding, and to show there are some
> board to add missing things like those above.

Yeah, I get that, but the point really is that you're not really adding
anything here except an empty device tree.

Maxime


signature.asc
Description: PGP signature

Re: [PATCH] dt-bindings: thermal: sun8i: Fix misplaced schema keyword in compatible strings

[Maxime Ripard]

On Tue, Feb 02, 2021 at 12:15:38PM -0600, Rob Herring wrote:
> A compatible string 'enum' mistakenly has 'const: ' in the compatible
> strings. Remove these.
> 
> Fixes: 0b28594d67a8 ("dt-bindings: thermal: Add YAML schema for sun8i-thermal 
> driver bindings")
> Cc: Vasily Khoruzhick 
> Cc: Yangtao Li 
> Cc: Zhang Rui 
> Cc: Daniel Lezcano 
> Cc: Amit Kucheria 
> Cc: Maxime Ripard 
> Cc: Chen-Yu Tsai 
> Cc: Jernej Skrabec 
> Cc: linux...@vger.kernel.org
> Signed-off-by: Rob Herring 

Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature

Re: [PATCH 1/3] dt-bindings: Fix undocumented compatible strings in examples

[Maxime Ripard]

On Tue, Feb 02, 2021 at 04:33:56PM -0800, Stephen Boyd wrote:
> Quoting Rob Herring (2021-02-02 12:55:42)
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/clock/allwinner,sun9i-a80-usb-clocks.yaml
> >  
> > b/Documentation/devicetree/bindings/clock/allwinner,sun9i-a80-usb-clocks.yaml
> > index fa0ee03a527f..53cc6df0df96 100644
> > --- 
> > a/Documentation/devicetree/bindings/clock/allwinner,sun9i-a80-usb-clocks.yaml
> > +++ 
> > b/Documentation/devicetree/bindings/clock/allwinner,sun9i-a80-usb-clocks.yaml
> > @@ -18,7 +18,7 @@ properties:
> >  const: 1
> >  
> >compatible:
> > -const: allwinner,sun9i-a80-usb-clocks
> > +const: allwinner,sun9i-a80-usb-clks
> 
> Should the file name change too?

Ideally yes, and with that change
Acked-by: Maxime Ripard 

Maxime


signature.asc
Description: PGP signature