All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
	"andrzej.hajda@intel.com" <andrzej.hajda@intel.com>,
	"neil.armstrong@linaro.org" <neil.armstrong@linaro.org>,
	"Laurent.pinchart@ideasonboard.com"
	<Laurent.pinchart@ideasonboard.com>,
	"jonas@kwiboo.se" <jonas@kwiboo.se>,
	"jernej.skrabec@gmail.com" <jernej.skrabec@gmail.com>,
	"airlied@gmail.com" <airlied@gmail.com>,
	"daniel@ffwll.ch" <daniel@ffwll.ch>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-phy@lists .infradead.org" <linux-phy@lists.infradead.org>,
	Sandor Yu <sandor.yu@nxp.com>
Cc: "kernel@pengutronix.de" <kernel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	Oliver Brown <oliver.brown@nxp.com>,
	"sam@ravnborg.org" <sam@ravnborg.org>
Subject: Re: [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver
Date: Mon, 16 Oct 2023 10:30:48 +0200	[thread overview]
Message-ID: <3258050.aeNJFYEL58@steina-w> (raw)
In-Reply-To: <PAXPR04MB9448AC9D8090E1F5F7E39C0EF4D7A@PAXPR04MB9448.eurprd04.prod.outlook.com>

Hi Sandor,

Am Montag, 16. Oktober 2023, 05:05:54 CEST schrieb Sandor Yu:
> Hi Alexander,
> 
> Thanks your comments,
> 
> > Hi Sandor,
> > 
> > thanks for the updated series.
> > 
> > Am Freitag, 13. Oktober 2023, 05:24:20 CEST schrieb Sandor Yu:
> > > MHDP8546 mailbox access functions will be share to other mhdp driver
> > > and Cadence HDP-TX HDMI/DP PHY drivers.
> > > Create a new mhdp helper driver and move all those functions into.
> > > 
> > > cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(), because
> > > it use the DPTX command ID DPTX_WRITE_REGISTER.
> > > 
> > > New cdns_mhdp_reg_write() is created with the general command ID
> > > GENERAL_REGISTER_WRITE.
> > > 
> > > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > > ---
> > > 
> > > v9->v10:
> > >  *use mhdp helper driver to replace macro functions,  move maibox
> > > 
> > > access function and mhdp hdmi/dp common API  functions into the
> > > driver.
> > > 
> > >  drivers/gpu/drm/bridge/cadence/Kconfig        |   4
> > >  drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
> > >  .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 306 ++++++++++++++
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 383 +++---------------
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  44 +-
> > >  include/drm/bridge/cdns-mhdp-helper.h         |  96 +++++
> > >  6 files changed, 473 insertions(+), 361 deletions(-)  create mode
> > > 
> > > 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > 
> > >  create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > > ec35215a20034..0b7b4626a7af0
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > > @@ -20,6 +20,9 @@ config DRM_CDNS_DSI_J721E
> > > 
> > >         the routing of the DSS DPI signal to the Cadence DSI.
> > >  
> > >  endif
> > > 
> > > +config CDNS_MHDP_HELPER
> > > +     tristate
> > > +
> > > 
> > >  config DRM_CDNS_MHDP8546
> > >  
> > >       tristate "Cadence DPI/DP bridge"
> > >       select DRM_DISPLAY_DP_HELPER
> > > 
> > > @@ -27,6 +30,7 @@ config DRM_CDNS_MHDP8546
> > > 
> > >       select DRM_DISPLAY_HELPER
> > >       select DRM_KMS_HELPER
> > >       select DRM_PANEL_BRIDGE
> > > 
> > > +     select CDNS_MHDP_HELPER
> > > 
> > >       depends on OF
> > >       help
> > >       
> > >         Support Cadence DPI to DP bridge. This is an internal diff
> > > 
> > > --git a/drivers/gpu/drm/bridge/cadence/Makefile
> > > b/drivers/gpu/drm/bridge/cadence/Makefile index
> > > c95fd5b81d137..087dc074820d7 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > > @@ -2,6 +2,7 @@
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o  cdns-dsi-y :=
> > > 
> > > cdns-dsi-core.o
> > > 
> > >  cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> > > 
> > > +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> > 
> > cdns-mhdp8546-y
> > 
> > > := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> > > :
> > >  cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) +=
> > > 
> > > cdns-mhdp8546-j721e.o diff --git
> > > a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file mode
> > > 100644 index 0000000000000..2e3eee40494f0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > @@ -0,0 +1,306 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + *
> > > + */
> > > +#include <drm/bridge/cdns-mhdp-helper.h> #include
> > > +<linux/dev_printk.h> #include <linux/module.h>
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base) {
> > > +     int ret, empty;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs +
> > 
> > CDNS_MAILBOX_EMPTY,
> > 
> > > +                              empty, !empty, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return readl(base->regs + CDNS_MAILBOX_RX_DATA) & 0xff; }
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_read);
> > 
> > No need to export this. You can make this function actually static.
> 
> OK, I will change it to static in the next version.
> 
> > > +
> > > +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val) {
> > > +     int ret, full;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_FULL,
> > > +                              full, !full, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     writel(val, base->regs + CDNS_MAILBOX_TX_DATA);
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_write);
> > 
> > No need to export that one as well. You can make this function actually
> > static as these two functions are only called from the helper itself.
> 
> mhdp8546 driver need this function.

True, cdns_mhdp_set_firmware_active() needs this. But this call seems a bit 
fishy. Couldn't this be written as follow?

---8<--
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/
gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 0582a5e42763f..c0364d05406cd 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -78,22 +78,16 @@ static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge 
*bridge)
 static
 int cdns_mhdp_set_firmware_active(struct cdns_mhdp_device *mhdp, bool enable)
 {
-       u8 msg[5];
+       u8 msg[1];
        int ret, i;
 
-       msg[0] = GENERAL_MAIN_CONTROL;
-       msg[1] = MB_MODULE_ID_GENERAL;
-       msg[2] = 0;
-       msg[3] = 1;
-       msg[4] = enable ? FW_ACTIVE : FW_STANDBY;
+       msg[0] = enable ? FW_ACTIVE : FW_STANDBY;
 
        mutex_lock(&mhdp->mbox_mutex);
 
-       for (i = 0; i < sizeof(msg); i++) {
-               ret = cdns_mhdp_mailbox_write(&mhdp->base, msg[i]);
-               if (ret)
-                       goto out;
-       }
+       ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
+                                    GENERAL_MAIN_CONTROL,
+                                    sizeof(msg), msg);
 
        /* read the firmware state */
        ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, msg, sizeof(msg));
---8<--

AFAICS cdns_mhdp_mailbox_send() is only a sequence of writes, writing 4 byte 
header and 1 byte data in this case.

Best regards,
Alexander

> 
> > > +
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode,
> > > +                               u16 req_size) {
> > > +     u32 mbox_size, i;
> > > +     u8 header[4];
> > > +     int ret;
> > > +
> > > +     /* read the header of the message */
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             header[i] = ret;
> > > +     }
> > > +
> > > +     mbox_size = get_unaligned_be16(header + 2);
> > > +
> > > +     if (opcode != header[0] || module_id != header[1] ||
> > > +         req_size != mbox_size) {
> > > +             /*
> > > +              * If the message in mailbox is not what we want, we
> > > + need
> > 
> > to
> > 
> > > +              * clear the mailbox by reading its contents.
> > > +              */
> > > +             for (i = 0; i < mbox_size; i++)
> > > +                     if (cdns_mhdp_mailbox_read(base) < 0)
> > > +                             break;
> > > +
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_header);
> > > +
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size) {
> > > +     u32 i;
> > > +     int ret;
> > > +
> > > +     for (i = 0; i < buff_size; i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             buff[i] = ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_data);
> > > +
> > > +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8
> > 
> > module_id,
> > 
> > > +                        u8 opcode, u16 size, u8 *message) {
> > > +     u8 header[4];
> > > +     int ret, i;
> > > +
> > > +     header[0] = opcode;
> > > +     header[1] = module_id;
> > > +     put_unaligned_be16(size, header + 2);
> > > +
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, header[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     for (i = 0; i < size; i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, message[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send);
> > > +
> > > +/* General helper functions */
> > > +int cdns_mhdp_reg_read(struct cdns_mhdp_base *base, u32 addr, u32
> > > +*value) {
> > > +     u8 msg[4], resp[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_READ,
> > > +                                  sizeof(msg), msg);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_GENERAL,
> > 
> > > +
> > 
> > GENERAL_REGISTER_READ,
> > 
> > > +                                         sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, resp, sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     /* Returned address value should be the same as requested */
> > > +     if (memcmp(msg, resp, sizeof(msg))) {
> > > +             ret = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > > +     *value = get_unaligned_be32(resp + 4);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +     if (ret) {
> > > +             dev_err(base->dev, "Failed to read register\n");
> > > +             *value = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_read);
> > > +
> > > +int cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32
> > > +val) {
> > > +     u8 msg[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +     put_unaligned_be32(val, msg + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_WRITE,
> > > +                                  sizeof(msg), msg);
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_write);
> > > +
> > > +/* DPTX helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val) {
> > > +     u8 msg[6];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, msg);
> > > +     put_unaligned_be32(val, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_REGISTER,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write);
> > > +
> > > +int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16 addr,
> > > +                            u8 start_bit, u8 bits_no, u32 val) {
> > > +     u8 field[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, field);
> > > +     field[2] = start_bit;
> > > +     field[3] = bits_no;
> > > +     put_unaligned_be32(val, field + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_FIELD,
> > 
> > sizeof(field),
> > field);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write_bit);
> > > +
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len) {
> > > +     u8 msg[5], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(len, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_READ_DPCD, sizeof(msg),
> > 
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_READ_DPCD,
> > > +                                         sizeof(reg) + len);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, data, len);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_read);
> > > +
> > > +int cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value) {
> > > +     u8 msg[6], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(1, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +     msg[5] = value;
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_DPCD,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_WRITE_DPCD,
> > 
> > sizeof(reg));
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     if (addr != get_unaligned_be24(reg + 2))
> > > +             ret = -EINVAL;
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     if (ret)
> > > +             dev_err(base->dev, "dpcd write failed: %d\n", ret);
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_write);
> > > +
> > > +MODULE_DESCRIPTION("Cadence MHDP Helper driver");
> > > +MODULE_AUTHOR("Sandor Yu <Sandor.yu@nxp.com>");
> > > +MODULE_LICENSE("GPL");
> > > [...]
> > > diff --git a/include/drm/bridge/cdns-mhdp-helper.h
> > > b/include/drm/bridge/cdns-mhdp-helper.h new file mode 100644 index
> > > 0000000000000..b89db9e842266
> > > --- /dev/null
> > > +++ b/include/drm/bridge/cdns-mhdp-helper.h
> > > @@ -0,0 +1,96 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + */
> > > +#ifndef __CDNS_MHDP_HELPER_H__
> > > +#define __CDNS_MHDP_HELPER_H__
> > > +
> > > +#include <asm/unaligned.h>
> > > +#include <linux/iopoll.h>
> > > +
> > > +/* mailbox regs offset */
> > > +#define CDNS_MAILBOX_FULL                    0x00008
> > > +#define CDNS_MAILBOX_EMPTY                   0x0000c
> > > +#define CDNS_MAILBOX_TX_DATA                 0x00010
> > > +#define CDNS_MAILBOX_RX_DATA                 0x00014
> > > +
> > > +#define MAILBOX_RETRY_US                     1000
> > > +#define MAILBOX_TIMEOUT_US                   2000000
> > > +
> > > +/* Module ID Code */
> > > +#define MB_MODULE_ID_DP_TX                   0x01
> > > +#define MB_MODULE_ID_HDMI_TX                 0x03
> > > +#define MB_MODULE_ID_HDCP_TX                 0x07
> > > +#define MB_MODULE_ID_HDCP_RX                 0x08
> > > +#define MB_MODULE_ID_HDCP_GENERAL            0x09
> > > +#define MB_MODULE_ID_GENERAL                 0x0A
> > > +
> > > +/* General Commands */
> > > +#define GENERAL_MAIN_CONTROL                 0x01
> > > +#define GENERAL_TEST_ECHO                    0x02
> > > +#define GENERAL_BUS_SETTINGS                 0x03
> > > +#define GENERAL_TEST_ACCESS                  0x04
> > > +#define GENERAL_REGISTER_WRITE                       0x05
> > > +#define GENERAL_WRITE_FIELD                  0x06
> > > +#define GENERAL_REGISTER_READ                        0x07
> > > +#define GENERAL_GET_HPD_STATE                        0x11
> > > +
> > > +/* DPTX Commands */
> > > +#define DPTX_SET_POWER_MNG                   0x00
> > > +#define DPTX_SET_HOST_CAPABILITIES           0x01
> > > +#define DPTX_GET_EDID                                0x02
> > > +#define DPTX_READ_DPCD                               0x03
> > > +#define DPTX_WRITE_DPCD                              0x04
> > > +#define DPTX_ENABLE_EVENT                    0x05
> > > +#define DPTX_WRITE_REGISTER                  0x06
> > > +#define DPTX_READ_REGISTER                   0x07
> > > +#define DPTX_WRITE_FIELD                     0x08
> > > +#define DPTX_TRAINING_CONTROL                        0x09
> > > +#define DPTX_READ_EVENT                              0x0a
> > > +#define DPTX_READ_LINK_STAT                  0x0b
> > > +#define DPTX_SET_VIDEO                               0x0c
> > > +#define DPTX_SET_AUDIO                               0x0d
> > > +#define DPTX_GET_LAST_AUX_STAUS                      0x0e
> > > +#define DPTX_SET_LINK_BREAK_POINT            0x0f
> > > +#define DPTX_FORCE_LANES                     0x10
> > > +#define DPTX_HPD_STATE                               0x11
> > > +#define DPTX_ADJUST_LT                               0x12
> > > +
> > > +/* HDMI TX Commands */
> > > +#define HDMI_TX_READ                         0x00
> > > +#define HDMI_TX_WRITE                                0x01
> > > +#define HDMI_TX_UPDATE_READ                  0x02
> > > +#define HDMI_TX_EDID                         0x03
> > > +#define HDMI_TX_EVENTS                               0x04
> > > +#define HDMI_TX_HPD_STATUS                   0x05
> > > +
> > > +struct cdns_mhdp_base {
> > > +     struct device *dev;
> > > +     void __iomem *regs;
> > > +     /* protect mailbox communications with the firmware */
> > > +     struct mutex *mbox_mutex;
> > > +};
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base); int
> > > +cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val);
> > 
> > You can remove these two declarations.
> 
> cdns_mhdp_mailbox_read will be removed. Thanks!
> 
> B.R
> Sandor
> 
> > Best regards,
> > Alexander
> > 
> > Best regards,
> > 
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode, u16
> > 
> > req_size);
> > 
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size); int
> > > +cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> > > +                        u8 opcode, u16 size, u8 *message);
> > > +
> > > +/* General commands helper functions */ int cdns_mhdp_reg_read(struct
> > > +cdns_mhdp_base *base, u32 addr, u32 *value); int
> > > +cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32 val);
> > > +
> > > +/* DPTX commands helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val); int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16
> > > +addr, +
> > > 
> > >       u8 start_bit, u8 bits_no, u32 val);
> > > 
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len); int
> > > +cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value);
> > > +
> > > +#endif /* __CDNS_MHDP_HELPER_H__ */
> > 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

WARNING: multiple messages have this Message-ID (diff)
From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
	"andrzej.hajda@intel.com" <andrzej.hajda@intel.com>,
	"neil.armstrong@linaro.org" <neil.armstrong@linaro.org>,
	"Laurent.pinchart@ideasonboard.com"
	<Laurent.pinchart@ideasonboard.com>,
	"jonas@kwiboo.se" <jonas@kwiboo.se>,
	"jernej.skrabec@gmail.com" <jernej.skrabec@gmail.com>,
	"airlied@gmail.com" <airlied@gmail.com>,
	"daniel@ffwll.ch" <daniel@ffwll.ch>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-phy@lists .infradead.org" <linux-phy@lists.infradead.org>,
	Sandor Yu <sandor.yu@nxp.com>
Cc: "kernel@pengutronix.de" <kernel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	Oliver Brown <oliver.brown@nxp.com>,
	"sam@ravnborg.org" <sam@ravnborg.org>
Subject: Re: [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver
Date: Mon, 16 Oct 2023 10:30:48 +0200	[thread overview]
Message-ID: <3258050.aeNJFYEL58@steina-w> (raw)
In-Reply-To: <PAXPR04MB9448AC9D8090E1F5F7E39C0EF4D7A@PAXPR04MB9448.eurprd04.prod.outlook.com>

Hi Sandor,

Am Montag, 16. Oktober 2023, 05:05:54 CEST schrieb Sandor Yu:
> Hi Alexander,
> 
> Thanks your comments,
> 
> > Hi Sandor,
> > 
> > thanks for the updated series.
> > 
> > Am Freitag, 13. Oktober 2023, 05:24:20 CEST schrieb Sandor Yu:
> > > MHDP8546 mailbox access functions will be share to other mhdp driver
> > > and Cadence HDP-TX HDMI/DP PHY drivers.
> > > Create a new mhdp helper driver and move all those functions into.
> > > 
> > > cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(), because
> > > it use the DPTX command ID DPTX_WRITE_REGISTER.
> > > 
> > > New cdns_mhdp_reg_write() is created with the general command ID
> > > GENERAL_REGISTER_WRITE.
> > > 
> > > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > > ---
> > > 
> > > v9->v10:
> > >  *use mhdp helper driver to replace macro functions,  move maibox
> > > 
> > > access function and mhdp hdmi/dp common API  functions into the
> > > driver.
> > > 
> > >  drivers/gpu/drm/bridge/cadence/Kconfig        |   4
> > >  drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
> > >  .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 306 ++++++++++++++
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 383 +++---------------
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  44 +-
> > >  include/drm/bridge/cdns-mhdp-helper.h         |  96 +++++
> > >  6 files changed, 473 insertions(+), 361 deletions(-)  create mode
> > > 
> > > 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > 
> > >  create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > > ec35215a20034..0b7b4626a7af0
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > > @@ -20,6 +20,9 @@ config DRM_CDNS_DSI_J721E
> > > 
> > >         the routing of the DSS DPI signal to the Cadence DSI.
> > >  
> > >  endif
> > > 
> > > +config CDNS_MHDP_HELPER
> > > +     tristate
> > > +
> > > 
> > >  config DRM_CDNS_MHDP8546
> > >  
> > >       tristate "Cadence DPI/DP bridge"
> > >       select DRM_DISPLAY_DP_HELPER
> > > 
> > > @@ -27,6 +30,7 @@ config DRM_CDNS_MHDP8546
> > > 
> > >       select DRM_DISPLAY_HELPER
> > >       select DRM_KMS_HELPER
> > >       select DRM_PANEL_BRIDGE
> > > 
> > > +     select CDNS_MHDP_HELPER
> > > 
> > >       depends on OF
> > >       help
> > >       
> > >         Support Cadence DPI to DP bridge. This is an internal diff
> > > 
> > > --git a/drivers/gpu/drm/bridge/cadence/Makefile
> > > b/drivers/gpu/drm/bridge/cadence/Makefile index
> > > c95fd5b81d137..087dc074820d7 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > > @@ -2,6 +2,7 @@
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o  cdns-dsi-y :=
> > > 
> > > cdns-dsi-core.o
> > > 
> > >  cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> > > 
> > > +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> > 
> > cdns-mhdp8546-y
> > 
> > > := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> > > :
> > >  cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) +=
> > > 
> > > cdns-mhdp8546-j721e.o diff --git
> > > a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file mode
> > > 100644 index 0000000000000..2e3eee40494f0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > @@ -0,0 +1,306 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + *
> > > + */
> > > +#include <drm/bridge/cdns-mhdp-helper.h> #include
> > > +<linux/dev_printk.h> #include <linux/module.h>
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base) {
> > > +     int ret, empty;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs +
> > 
> > CDNS_MAILBOX_EMPTY,
> > 
> > > +                              empty, !empty, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return readl(base->regs + CDNS_MAILBOX_RX_DATA) & 0xff; }
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_read);
> > 
> > No need to export this. You can make this function actually static.
> 
> OK, I will change it to static in the next version.
> 
> > > +
> > > +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val) {
> > > +     int ret, full;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_FULL,
> > > +                              full, !full, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     writel(val, base->regs + CDNS_MAILBOX_TX_DATA);
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_write);
> > 
> > No need to export that one as well. You can make this function actually
> > static as these two functions are only called from the helper itself.
> 
> mhdp8546 driver need this function.

True, cdns_mhdp_set_firmware_active() needs this. But this call seems a bit 
fishy. Couldn't this be written as follow?

---8<--
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/
gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 0582a5e42763f..c0364d05406cd 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -78,22 +78,16 @@ static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge 
*bridge)
 static
 int cdns_mhdp_set_firmware_active(struct cdns_mhdp_device *mhdp, bool enable)
 {
-       u8 msg[5];
+       u8 msg[1];
        int ret, i;
 
-       msg[0] = GENERAL_MAIN_CONTROL;
-       msg[1] = MB_MODULE_ID_GENERAL;
-       msg[2] = 0;
-       msg[3] = 1;
-       msg[4] = enable ? FW_ACTIVE : FW_STANDBY;
+       msg[0] = enable ? FW_ACTIVE : FW_STANDBY;
 
        mutex_lock(&mhdp->mbox_mutex);
 
-       for (i = 0; i < sizeof(msg); i++) {
-               ret = cdns_mhdp_mailbox_write(&mhdp->base, msg[i]);
-               if (ret)
-                       goto out;
-       }
+       ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
+                                    GENERAL_MAIN_CONTROL,
+                                    sizeof(msg), msg);
 
        /* read the firmware state */
        ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, msg, sizeof(msg));
---8<--

AFAICS cdns_mhdp_mailbox_send() is only a sequence of writes, writing 4 byte 
header and 1 byte data in this case.

Best regards,
Alexander

> 
> > > +
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode,
> > > +                               u16 req_size) {
> > > +     u32 mbox_size, i;
> > > +     u8 header[4];
> > > +     int ret;
> > > +
> > > +     /* read the header of the message */
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             header[i] = ret;
> > > +     }
> > > +
> > > +     mbox_size = get_unaligned_be16(header + 2);
> > > +
> > > +     if (opcode != header[0] || module_id != header[1] ||
> > > +         req_size != mbox_size) {
> > > +             /*
> > > +              * If the message in mailbox is not what we want, we
> > > + need
> > 
> > to
> > 
> > > +              * clear the mailbox by reading its contents.
> > > +              */
> > > +             for (i = 0; i < mbox_size; i++)
> > > +                     if (cdns_mhdp_mailbox_read(base) < 0)
> > > +                             break;
> > > +
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_header);
> > > +
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size) {
> > > +     u32 i;
> > > +     int ret;
> > > +
> > > +     for (i = 0; i < buff_size; i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             buff[i] = ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_data);
> > > +
> > > +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8
> > 
> > module_id,
> > 
> > > +                        u8 opcode, u16 size, u8 *message) {
> > > +     u8 header[4];
> > > +     int ret, i;
> > > +
> > > +     header[0] = opcode;
> > > +     header[1] = module_id;
> > > +     put_unaligned_be16(size, header + 2);
> > > +
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, header[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     for (i = 0; i < size; i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, message[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send);
> > > +
> > > +/* General helper functions */
> > > +int cdns_mhdp_reg_read(struct cdns_mhdp_base *base, u32 addr, u32
> > > +*value) {
> > > +     u8 msg[4], resp[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_READ,
> > > +                                  sizeof(msg), msg);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_GENERAL,
> > 
> > > +
> > 
> > GENERAL_REGISTER_READ,
> > 
> > > +                                         sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, resp, sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     /* Returned address value should be the same as requested */
> > > +     if (memcmp(msg, resp, sizeof(msg))) {
> > > +             ret = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > > +     *value = get_unaligned_be32(resp + 4);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +     if (ret) {
> > > +             dev_err(base->dev, "Failed to read register\n");
> > > +             *value = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_read);
> > > +
> > > +int cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32
> > > +val) {
> > > +     u8 msg[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +     put_unaligned_be32(val, msg + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_WRITE,
> > > +                                  sizeof(msg), msg);
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_write);
> > > +
> > > +/* DPTX helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val) {
> > > +     u8 msg[6];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, msg);
> > > +     put_unaligned_be32(val, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_REGISTER,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write);
> > > +
> > > +int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16 addr,
> > > +                            u8 start_bit, u8 bits_no, u32 val) {
> > > +     u8 field[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, field);
> > > +     field[2] = start_bit;
> > > +     field[3] = bits_no;
> > > +     put_unaligned_be32(val, field + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_FIELD,
> > 
> > sizeof(field),
> > field);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write_bit);
> > > +
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len) {
> > > +     u8 msg[5], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(len, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_READ_DPCD, sizeof(msg),
> > 
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_READ_DPCD,
> > > +                                         sizeof(reg) + len);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, data, len);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_read);
> > > +
> > > +int cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value) {
> > > +     u8 msg[6], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(1, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +     msg[5] = value;
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_DPCD,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_WRITE_DPCD,
> > 
> > sizeof(reg));
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     if (addr != get_unaligned_be24(reg + 2))
> > > +             ret = -EINVAL;
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     if (ret)
> > > +             dev_err(base->dev, "dpcd write failed: %d\n", ret);
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_write);
> > > +
> > > +MODULE_DESCRIPTION("Cadence MHDP Helper driver");
> > > +MODULE_AUTHOR("Sandor Yu <Sandor.yu@nxp.com>");
> > > +MODULE_LICENSE("GPL");
> > > [...]
> > > diff --git a/include/drm/bridge/cdns-mhdp-helper.h
> > > b/include/drm/bridge/cdns-mhdp-helper.h new file mode 100644 index
> > > 0000000000000..b89db9e842266
> > > --- /dev/null
> > > +++ b/include/drm/bridge/cdns-mhdp-helper.h
> > > @@ -0,0 +1,96 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + */
> > > +#ifndef __CDNS_MHDP_HELPER_H__
> > > +#define __CDNS_MHDP_HELPER_H__
> > > +
> > > +#include <asm/unaligned.h>
> > > +#include <linux/iopoll.h>
> > > +
> > > +/* mailbox regs offset */
> > > +#define CDNS_MAILBOX_FULL                    0x00008
> > > +#define CDNS_MAILBOX_EMPTY                   0x0000c
> > > +#define CDNS_MAILBOX_TX_DATA                 0x00010
> > > +#define CDNS_MAILBOX_RX_DATA                 0x00014
> > > +
> > > +#define MAILBOX_RETRY_US                     1000
> > > +#define MAILBOX_TIMEOUT_US                   2000000
> > > +
> > > +/* Module ID Code */
> > > +#define MB_MODULE_ID_DP_TX                   0x01
> > > +#define MB_MODULE_ID_HDMI_TX                 0x03
> > > +#define MB_MODULE_ID_HDCP_TX                 0x07
> > > +#define MB_MODULE_ID_HDCP_RX                 0x08
> > > +#define MB_MODULE_ID_HDCP_GENERAL            0x09
> > > +#define MB_MODULE_ID_GENERAL                 0x0A
> > > +
> > > +/* General Commands */
> > > +#define GENERAL_MAIN_CONTROL                 0x01
> > > +#define GENERAL_TEST_ECHO                    0x02
> > > +#define GENERAL_BUS_SETTINGS                 0x03
> > > +#define GENERAL_TEST_ACCESS                  0x04
> > > +#define GENERAL_REGISTER_WRITE                       0x05
> > > +#define GENERAL_WRITE_FIELD                  0x06
> > > +#define GENERAL_REGISTER_READ                        0x07
> > > +#define GENERAL_GET_HPD_STATE                        0x11
> > > +
> > > +/* DPTX Commands */
> > > +#define DPTX_SET_POWER_MNG                   0x00
> > > +#define DPTX_SET_HOST_CAPABILITIES           0x01
> > > +#define DPTX_GET_EDID                                0x02
> > > +#define DPTX_READ_DPCD                               0x03
> > > +#define DPTX_WRITE_DPCD                              0x04
> > > +#define DPTX_ENABLE_EVENT                    0x05
> > > +#define DPTX_WRITE_REGISTER                  0x06
> > > +#define DPTX_READ_REGISTER                   0x07
> > > +#define DPTX_WRITE_FIELD                     0x08
> > > +#define DPTX_TRAINING_CONTROL                        0x09
> > > +#define DPTX_READ_EVENT                              0x0a
> > > +#define DPTX_READ_LINK_STAT                  0x0b
> > > +#define DPTX_SET_VIDEO                               0x0c
> > > +#define DPTX_SET_AUDIO                               0x0d
> > > +#define DPTX_GET_LAST_AUX_STAUS                      0x0e
> > > +#define DPTX_SET_LINK_BREAK_POINT            0x0f
> > > +#define DPTX_FORCE_LANES                     0x10
> > > +#define DPTX_HPD_STATE                               0x11
> > > +#define DPTX_ADJUST_LT                               0x12
> > > +
> > > +/* HDMI TX Commands */
> > > +#define HDMI_TX_READ                         0x00
> > > +#define HDMI_TX_WRITE                                0x01
> > > +#define HDMI_TX_UPDATE_READ                  0x02
> > > +#define HDMI_TX_EDID                         0x03
> > > +#define HDMI_TX_EVENTS                               0x04
> > > +#define HDMI_TX_HPD_STATUS                   0x05
> > > +
> > > +struct cdns_mhdp_base {
> > > +     struct device *dev;
> > > +     void __iomem *regs;
> > > +     /* protect mailbox communications with the firmware */
> > > +     struct mutex *mbox_mutex;
> > > +};
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base); int
> > > +cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val);
> > 
> > You can remove these two declarations.
> 
> cdns_mhdp_mailbox_read will be removed. Thanks!
> 
> B.R
> Sandor
> 
> > Best regards,
> > Alexander
> > 
> > Best regards,
> > 
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode, u16
> > 
> > req_size);
> > 
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size); int
> > > +cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> > > +                        u8 opcode, u16 size, u8 *message);
> > > +
> > > +/* General commands helper functions */ int cdns_mhdp_reg_read(struct
> > > +cdns_mhdp_base *base, u32 addr, u32 *value); int
> > > +cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32 val);
> > > +
> > > +/* DPTX commands helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val); int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16
> > > +addr, +
> > > 
> > >       u8 start_bit, u8 bits_no, u32 val);
> > > 
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len); int
> > > +cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value);
> > > +
> > > +#endif /* __CDNS_MHDP_HELPER_H__ */
> > 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



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

WARNING: multiple messages have this Message-ID (diff)
From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
	"andrzej.hajda@intel.com" <andrzej.hajda@intel.com>,
	"neil.armstrong@linaro.org" <neil.armstrong@linaro.org>,
	"Laurent.pinchart@ideasonboard.com"
	<Laurent.pinchart@ideasonboard.com>,
	"jonas@kwiboo.se" <jonas@kwiboo.se>,
	"jernej.skrabec@gmail.com" <jernej.skrabec@gmail.com>,
	"airlied@gmail.com" <airlied@gmail.com>,
	"daniel@ffwll.ch" <daniel@ffwll.ch>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-phy@lists .infradead.org" <linux-phy@lists.infradead.org>,
	Sandor Yu <sandor.yu@nxp.com>
Cc: "kernel@pengutronix.de" <kernel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	Oliver Brown <oliver.brown@nxp.com>,
	"sam@ravnborg.org" <sam@ravnborg.org>
Subject: Re: [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver
Date: Mon, 16 Oct 2023 10:30:48 +0200	[thread overview]
Message-ID: <3258050.aeNJFYEL58@steina-w> (raw)
In-Reply-To: <PAXPR04MB9448AC9D8090E1F5F7E39C0EF4D7A@PAXPR04MB9448.eurprd04.prod.outlook.com>

Hi Sandor,

Am Montag, 16. Oktober 2023, 05:05:54 CEST schrieb Sandor Yu:
> Hi Alexander,
> 
> Thanks your comments,
> 
> > Hi Sandor,
> > 
> > thanks for the updated series.
> > 
> > Am Freitag, 13. Oktober 2023, 05:24:20 CEST schrieb Sandor Yu:
> > > MHDP8546 mailbox access functions will be share to other mhdp driver
> > > and Cadence HDP-TX HDMI/DP PHY drivers.
> > > Create a new mhdp helper driver and move all those functions into.
> > > 
> > > cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(), because
> > > it use the DPTX command ID DPTX_WRITE_REGISTER.
> > > 
> > > New cdns_mhdp_reg_write() is created with the general command ID
> > > GENERAL_REGISTER_WRITE.
> > > 
> > > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > > ---
> > > 
> > > v9->v10:
> > >  *use mhdp helper driver to replace macro functions,  move maibox
> > > 
> > > access function and mhdp hdmi/dp common API  functions into the
> > > driver.
> > > 
> > >  drivers/gpu/drm/bridge/cadence/Kconfig        |   4
> > >  drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
> > >  .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 306 ++++++++++++++
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 383 +++---------------
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  44 +-
> > >  include/drm/bridge/cdns-mhdp-helper.h         |  96 +++++
> > >  6 files changed, 473 insertions(+), 361 deletions(-)  create mode
> > > 
> > > 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > 
> > >  create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > > ec35215a20034..0b7b4626a7af0
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > > @@ -20,6 +20,9 @@ config DRM_CDNS_DSI_J721E
> > > 
> > >         the routing of the DSS DPI signal to the Cadence DSI.
> > >  
> > >  endif
> > > 
> > > +config CDNS_MHDP_HELPER
> > > +     tristate
> > > +
> > > 
> > >  config DRM_CDNS_MHDP8546
> > >  
> > >       tristate "Cadence DPI/DP bridge"
> > >       select DRM_DISPLAY_DP_HELPER
> > > 
> > > @@ -27,6 +30,7 @@ config DRM_CDNS_MHDP8546
> > > 
> > >       select DRM_DISPLAY_HELPER
> > >       select DRM_KMS_HELPER
> > >       select DRM_PANEL_BRIDGE
> > > 
> > > +     select CDNS_MHDP_HELPER
> > > 
> > >       depends on OF
> > >       help
> > >       
> > >         Support Cadence DPI to DP bridge. This is an internal diff
> > > 
> > > --git a/drivers/gpu/drm/bridge/cadence/Makefile
> > > b/drivers/gpu/drm/bridge/cadence/Makefile index
> > > c95fd5b81d137..087dc074820d7 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > > @@ -2,6 +2,7 @@
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o  cdns-dsi-y :=
> > > 
> > > cdns-dsi-core.o
> > > 
> > >  cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> > > 
> > > +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> > 
> > cdns-mhdp8546-y
> > 
> > > := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> > > :
> > >  cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) +=
> > > 
> > > cdns-mhdp8546-j721e.o diff --git
> > > a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file mode
> > > 100644 index 0000000000000..2e3eee40494f0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > @@ -0,0 +1,306 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + *
> > > + */
> > > +#include <drm/bridge/cdns-mhdp-helper.h> #include
> > > +<linux/dev_printk.h> #include <linux/module.h>
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base) {
> > > +     int ret, empty;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs +
> > 
> > CDNS_MAILBOX_EMPTY,
> > 
> > > +                              empty, !empty, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return readl(base->regs + CDNS_MAILBOX_RX_DATA) & 0xff; }
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_read);
> > 
> > No need to export this. You can make this function actually static.
> 
> OK, I will change it to static in the next version.
> 
> > > +
> > > +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val) {
> > > +     int ret, full;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_FULL,
> > > +                              full, !full, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     writel(val, base->regs + CDNS_MAILBOX_TX_DATA);
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_write);
> > 
> > No need to export that one as well. You can make this function actually
> > static as these two functions are only called from the helper itself.
> 
> mhdp8546 driver need this function.

True, cdns_mhdp_set_firmware_active() needs this. But this call seems a bit 
fishy. Couldn't this be written as follow?

---8<--
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/
gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 0582a5e42763f..c0364d05406cd 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -78,22 +78,16 @@ static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge 
*bridge)
 static
 int cdns_mhdp_set_firmware_active(struct cdns_mhdp_device *mhdp, bool enable)
 {
-       u8 msg[5];
+       u8 msg[1];
        int ret, i;
 
-       msg[0] = GENERAL_MAIN_CONTROL;
-       msg[1] = MB_MODULE_ID_GENERAL;
-       msg[2] = 0;
-       msg[3] = 1;
-       msg[4] = enable ? FW_ACTIVE : FW_STANDBY;
+       msg[0] = enable ? FW_ACTIVE : FW_STANDBY;
 
        mutex_lock(&mhdp->mbox_mutex);
 
-       for (i = 0; i < sizeof(msg); i++) {
-               ret = cdns_mhdp_mailbox_write(&mhdp->base, msg[i]);
-               if (ret)
-                       goto out;
-       }
+       ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
+                                    GENERAL_MAIN_CONTROL,
+                                    sizeof(msg), msg);
 
        /* read the firmware state */
        ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, msg, sizeof(msg));
---8<--

AFAICS cdns_mhdp_mailbox_send() is only a sequence of writes, writing 4 byte 
header and 1 byte data in this case.

Best regards,
Alexander

> 
> > > +
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode,
> > > +                               u16 req_size) {
> > > +     u32 mbox_size, i;
> > > +     u8 header[4];
> > > +     int ret;
> > > +
> > > +     /* read the header of the message */
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             header[i] = ret;
> > > +     }
> > > +
> > > +     mbox_size = get_unaligned_be16(header + 2);
> > > +
> > > +     if (opcode != header[0] || module_id != header[1] ||
> > > +         req_size != mbox_size) {
> > > +             /*
> > > +              * If the message in mailbox is not what we want, we
> > > + need
> > 
> > to
> > 
> > > +              * clear the mailbox by reading its contents.
> > > +              */
> > > +             for (i = 0; i < mbox_size; i++)
> > > +                     if (cdns_mhdp_mailbox_read(base) < 0)
> > > +                             break;
> > > +
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_header);
> > > +
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size) {
> > > +     u32 i;
> > > +     int ret;
> > > +
> > > +     for (i = 0; i < buff_size; i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             buff[i] = ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_data);
> > > +
> > > +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8
> > 
> > module_id,
> > 
> > > +                        u8 opcode, u16 size, u8 *message) {
> > > +     u8 header[4];
> > > +     int ret, i;
> > > +
> > > +     header[0] = opcode;
> > > +     header[1] = module_id;
> > > +     put_unaligned_be16(size, header + 2);
> > > +
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, header[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     for (i = 0; i < size; i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, message[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send);
> > > +
> > > +/* General helper functions */
> > > +int cdns_mhdp_reg_read(struct cdns_mhdp_base *base, u32 addr, u32
> > > +*value) {
> > > +     u8 msg[4], resp[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_READ,
> > > +                                  sizeof(msg), msg);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_GENERAL,
> > 
> > > +
> > 
> > GENERAL_REGISTER_READ,
> > 
> > > +                                         sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, resp, sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     /* Returned address value should be the same as requested */
> > > +     if (memcmp(msg, resp, sizeof(msg))) {
> > > +             ret = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > > +     *value = get_unaligned_be32(resp + 4);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +     if (ret) {
> > > +             dev_err(base->dev, "Failed to read register\n");
> > > +             *value = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_read);
> > > +
> > > +int cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32
> > > +val) {
> > > +     u8 msg[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +     put_unaligned_be32(val, msg + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_WRITE,
> > > +                                  sizeof(msg), msg);
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_write);
> > > +
> > > +/* DPTX helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val) {
> > > +     u8 msg[6];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, msg);
> > > +     put_unaligned_be32(val, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_REGISTER,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write);
> > > +
> > > +int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16 addr,
> > > +                            u8 start_bit, u8 bits_no, u32 val) {
> > > +     u8 field[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, field);
> > > +     field[2] = start_bit;
> > > +     field[3] = bits_no;
> > > +     put_unaligned_be32(val, field + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_FIELD,
> > 
> > sizeof(field),
> > field);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write_bit);
> > > +
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len) {
> > > +     u8 msg[5], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(len, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_READ_DPCD, sizeof(msg),
> > 
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_READ_DPCD,
> > > +                                         sizeof(reg) + len);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, data, len);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_read);
> > > +
> > > +int cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value) {
> > > +     u8 msg[6], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(1, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +     msg[5] = value;
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_DPCD,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_WRITE_DPCD,
> > 
> > sizeof(reg));
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     if (addr != get_unaligned_be24(reg + 2))
> > > +             ret = -EINVAL;
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     if (ret)
> > > +             dev_err(base->dev, "dpcd write failed: %d\n", ret);
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_write);
> > > +
> > > +MODULE_DESCRIPTION("Cadence MHDP Helper driver");
> > > +MODULE_AUTHOR("Sandor Yu <Sandor.yu@nxp.com>");
> > > +MODULE_LICENSE("GPL");
> > > [...]
> > > diff --git a/include/drm/bridge/cdns-mhdp-helper.h
> > > b/include/drm/bridge/cdns-mhdp-helper.h new file mode 100644 index
> > > 0000000000000..b89db9e842266
> > > --- /dev/null
> > > +++ b/include/drm/bridge/cdns-mhdp-helper.h
> > > @@ -0,0 +1,96 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + */
> > > +#ifndef __CDNS_MHDP_HELPER_H__
> > > +#define __CDNS_MHDP_HELPER_H__
> > > +
> > > +#include <asm/unaligned.h>
> > > +#include <linux/iopoll.h>
> > > +
> > > +/* mailbox regs offset */
> > > +#define CDNS_MAILBOX_FULL                    0x00008
> > > +#define CDNS_MAILBOX_EMPTY                   0x0000c
> > > +#define CDNS_MAILBOX_TX_DATA                 0x00010
> > > +#define CDNS_MAILBOX_RX_DATA                 0x00014
> > > +
> > > +#define MAILBOX_RETRY_US                     1000
> > > +#define MAILBOX_TIMEOUT_US                   2000000
> > > +
> > > +/* Module ID Code */
> > > +#define MB_MODULE_ID_DP_TX                   0x01
> > > +#define MB_MODULE_ID_HDMI_TX                 0x03
> > > +#define MB_MODULE_ID_HDCP_TX                 0x07
> > > +#define MB_MODULE_ID_HDCP_RX                 0x08
> > > +#define MB_MODULE_ID_HDCP_GENERAL            0x09
> > > +#define MB_MODULE_ID_GENERAL                 0x0A
> > > +
> > > +/* General Commands */
> > > +#define GENERAL_MAIN_CONTROL                 0x01
> > > +#define GENERAL_TEST_ECHO                    0x02
> > > +#define GENERAL_BUS_SETTINGS                 0x03
> > > +#define GENERAL_TEST_ACCESS                  0x04
> > > +#define GENERAL_REGISTER_WRITE                       0x05
> > > +#define GENERAL_WRITE_FIELD                  0x06
> > > +#define GENERAL_REGISTER_READ                        0x07
> > > +#define GENERAL_GET_HPD_STATE                        0x11
> > > +
> > > +/* DPTX Commands */
> > > +#define DPTX_SET_POWER_MNG                   0x00
> > > +#define DPTX_SET_HOST_CAPABILITIES           0x01
> > > +#define DPTX_GET_EDID                                0x02
> > > +#define DPTX_READ_DPCD                               0x03
> > > +#define DPTX_WRITE_DPCD                              0x04
> > > +#define DPTX_ENABLE_EVENT                    0x05
> > > +#define DPTX_WRITE_REGISTER                  0x06
> > > +#define DPTX_READ_REGISTER                   0x07
> > > +#define DPTX_WRITE_FIELD                     0x08
> > > +#define DPTX_TRAINING_CONTROL                        0x09
> > > +#define DPTX_READ_EVENT                              0x0a
> > > +#define DPTX_READ_LINK_STAT                  0x0b
> > > +#define DPTX_SET_VIDEO                               0x0c
> > > +#define DPTX_SET_AUDIO                               0x0d
> > > +#define DPTX_GET_LAST_AUX_STAUS                      0x0e
> > > +#define DPTX_SET_LINK_BREAK_POINT            0x0f
> > > +#define DPTX_FORCE_LANES                     0x10
> > > +#define DPTX_HPD_STATE                               0x11
> > > +#define DPTX_ADJUST_LT                               0x12
> > > +
> > > +/* HDMI TX Commands */
> > > +#define HDMI_TX_READ                         0x00
> > > +#define HDMI_TX_WRITE                                0x01
> > > +#define HDMI_TX_UPDATE_READ                  0x02
> > > +#define HDMI_TX_EDID                         0x03
> > > +#define HDMI_TX_EVENTS                               0x04
> > > +#define HDMI_TX_HPD_STATUS                   0x05
> > > +
> > > +struct cdns_mhdp_base {
> > > +     struct device *dev;
> > > +     void __iomem *regs;
> > > +     /* protect mailbox communications with the firmware */
> > > +     struct mutex *mbox_mutex;
> > > +};
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base); int
> > > +cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val);
> > 
> > You can remove these two declarations.
> 
> cdns_mhdp_mailbox_read will be removed. Thanks!
> 
> B.R
> Sandor
> 
> > Best regards,
> > Alexander
> > 
> > Best regards,
> > 
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode, u16
> > 
> > req_size);
> > 
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size); int
> > > +cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> > > +                        u8 opcode, u16 size, u8 *message);
> > > +
> > > +/* General commands helper functions */ int cdns_mhdp_reg_read(struct
> > > +cdns_mhdp_base *base, u32 addr, u32 *value); int
> > > +cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32 val);
> > > +
> > > +/* DPTX commands helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val); int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16
> > > +addr, +
> > > 
> > >       u8 start_bit, u8 bits_no, u32 val);
> > > 
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len); int
> > > +cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value);
> > > +
> > > +#endif /* __CDNS_MHDP_HELPER_H__ */
> > 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



WARNING: multiple messages have this Message-ID (diff)
From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
	"andrzej.hajda@intel.com" <andrzej.hajda@intel.com>,
	"neil.armstrong@linaro.org" <neil.armstrong@linaro.org>,
	"Laurent.pinchart@ideasonboard.com"
	<Laurent.pinchart@ideasonboard.com>,
	"jonas@kwiboo.se" <jonas@kwiboo.se>,
	"jernej.skrabec@gmail.com" <jernej.skrabec@gmail.com>,
	"airlied@gmail.com" <airlied@gmail.com>,
	"daniel@ffwll.ch" <daniel@ffwll.ch>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-phy@lists .infradead.org" <linux-phy@lists.infradead.org>,
	Sandor Yu <sandor.yu@nxp.com>
Cc: Oliver Brown <oliver.brown@nxp.com>,
	"sam@ravnborg.org" <sam@ravnborg.org>,
	dl-linux-imx <linux-imx@nxp.com>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: Re: [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver
Date: Mon, 16 Oct 2023 10:30:48 +0200	[thread overview]
Message-ID: <3258050.aeNJFYEL58@steina-w> (raw)
In-Reply-To: <PAXPR04MB9448AC9D8090E1F5F7E39C0EF4D7A@PAXPR04MB9448.eurprd04.prod.outlook.com>

Hi Sandor,

Am Montag, 16. Oktober 2023, 05:05:54 CEST schrieb Sandor Yu:
> Hi Alexander,
> 
> Thanks your comments,
> 
> > Hi Sandor,
> > 
> > thanks for the updated series.
> > 
> > Am Freitag, 13. Oktober 2023, 05:24:20 CEST schrieb Sandor Yu:
> > > MHDP8546 mailbox access functions will be share to other mhdp driver
> > > and Cadence HDP-TX HDMI/DP PHY drivers.
> > > Create a new mhdp helper driver and move all those functions into.
> > > 
> > > cdns_mhdp_reg_write() is renamed to cdns_mhdp_dp_reg_write(), because
> > > it use the DPTX command ID DPTX_WRITE_REGISTER.
> > > 
> > > New cdns_mhdp_reg_write() is created with the general command ID
> > > GENERAL_REGISTER_WRITE.
> > > 
> > > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > > ---
> > > 
> > > v9->v10:
> > >  *use mhdp helper driver to replace macro functions,  move maibox
> > > 
> > > access function and mhdp hdmi/dp common API  functions into the
> > > driver.
> > > 
> > >  drivers/gpu/drm/bridge/cadence/Kconfig        |   4
> > >  drivers/gpu/drm/bridge/cadence/Makefile       |   1 +
> > >  .../gpu/drm/bridge/cadence/cdns-mhdp-helper.c | 306 ++++++++++++++
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 383 +++---------------
> > >  .../drm/bridge/cadence/cdns-mhdp8546-core.h   |  44 +-
> > >  include/drm/bridge/cdns-mhdp-helper.h         |  96 +++++
> > >  6 files changed, 473 insertions(+), 361 deletions(-)  create mode
> > > 
> > > 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > 
> > >  create mode 100644 include/drm/bridge/cdns-mhdp-helper.h
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > > ec35215a20034..0b7b4626a7af0
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > > @@ -20,6 +20,9 @@ config DRM_CDNS_DSI_J721E
> > > 
> > >         the routing of the DSS DPI signal to the Cadence DSI.
> > >  
> > >  endif
> > > 
> > > +config CDNS_MHDP_HELPER
> > > +     tristate
> > > +
> > > 
> > >  config DRM_CDNS_MHDP8546
> > >  
> > >       tristate "Cadence DPI/DP bridge"
> > >       select DRM_DISPLAY_DP_HELPER
> > > 
> > > @@ -27,6 +30,7 @@ config DRM_CDNS_MHDP8546
> > > 
> > >       select DRM_DISPLAY_HELPER
> > >       select DRM_KMS_HELPER
> > >       select DRM_PANEL_BRIDGE
> > > 
> > > +     select CDNS_MHDP_HELPER
> > > 
> > >       depends on OF
> > >       help
> > >       
> > >         Support Cadence DPI to DP bridge. This is an internal diff
> > > 
> > > --git a/drivers/gpu/drm/bridge/cadence/Makefile
> > > b/drivers/gpu/drm/bridge/cadence/Makefile index
> > > c95fd5b81d137..087dc074820d7 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > > @@ -2,6 +2,7 @@
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o  cdns-dsi-y :=
> > > 
> > > cdns-dsi-core.o
> > > 
> > >  cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) += cdns-dsi-j721e.o
> > > 
> > > +obj-$(CONFIG_CDNS_MHDP_HELPER) += cdns-mhdp-helper.o
> > > 
> > >  obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> > 
> > cdns-mhdp8546-y
> > 
> > > := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> > > :
> > >  cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) +=
> > > 
> > > cdns-mhdp8546-j721e.o diff --git
> > > a/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c new file mode
> > > 100644 index 0000000000000..2e3eee40494f0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp-helper.c
> > > @@ -0,0 +1,306 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + *
> > > + */
> > > +#include <drm/bridge/cdns-mhdp-helper.h> #include
> > > +<linux/dev_printk.h> #include <linux/module.h>
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base) {
> > > +     int ret, empty;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs +
> > 
> > CDNS_MAILBOX_EMPTY,
> > 
> > > +                              empty, !empty, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return readl(base->regs + CDNS_MAILBOX_RX_DATA) & 0xff; }
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_read);
> > 
> > No need to export this. You can make this function actually static.
> 
> OK, I will change it to static in the next version.
> 
> > > +
> > > +int cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val) {
> > > +     int ret, full;
> > > +
> > > +     WARN_ON(!mutex_is_locked(base->mbox_mutex));
> > > +
> > > +     ret = readx_poll_timeout(readl, base->regs + CDNS_MAILBOX_FULL,
> > > +                              full, !full, MAILBOX_RETRY_US,
> > > +                              MAILBOX_TIMEOUT_US);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     writel(val, base->regs + CDNS_MAILBOX_TX_DATA);
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_write);
> > 
> > No need to export that one as well. You can make this function actually
> > static as these two functions are only called from the helper itself.
> 
> mhdp8546 driver need this function.

True, cdns_mhdp_set_firmware_active() needs this. But this call seems a bit 
fishy. Couldn't this be written as follow?

---8<--
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/
gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 0582a5e42763f..c0364d05406cd 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -78,22 +78,16 @@ static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge 
*bridge)
 static
 int cdns_mhdp_set_firmware_active(struct cdns_mhdp_device *mhdp, bool enable)
 {
-       u8 msg[5];
+       u8 msg[1];
        int ret, i;
 
-       msg[0] = GENERAL_MAIN_CONTROL;
-       msg[1] = MB_MODULE_ID_GENERAL;
-       msg[2] = 0;
-       msg[3] = 1;
-       msg[4] = enable ? FW_ACTIVE : FW_STANDBY;
+       msg[0] = enable ? FW_ACTIVE : FW_STANDBY;
 
        mutex_lock(&mhdp->mbox_mutex);
 
-       for (i = 0; i < sizeof(msg); i++) {
-               ret = cdns_mhdp_mailbox_write(&mhdp->base, msg[i]);
-               if (ret)
-                       goto out;
-       }
+       ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
+                                    GENERAL_MAIN_CONTROL,
+                                    sizeof(msg), msg);
 
        /* read the firmware state */
        ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, msg, sizeof(msg));
---8<--

AFAICS cdns_mhdp_mailbox_send() is only a sequence of writes, writing 4 byte 
header and 1 byte data in this case.

Best regards,
Alexander

> 
> > > +
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode,
> > > +                               u16 req_size) {
> > > +     u32 mbox_size, i;
> > > +     u8 header[4];
> > > +     int ret;
> > > +
> > > +     /* read the header of the message */
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             header[i] = ret;
> > > +     }
> > > +
> > > +     mbox_size = get_unaligned_be16(header + 2);
> > > +
> > > +     if (opcode != header[0] || module_id != header[1] ||
> > > +         req_size != mbox_size) {
> > > +             /*
> > > +              * If the message in mailbox is not what we want, we
> > > + need
> > 
> > to
> > 
> > > +              * clear the mailbox by reading its contents.
> > > +              */
> > > +             for (i = 0; i < mbox_size; i++)
> > > +                     if (cdns_mhdp_mailbox_read(base) < 0)
> > > +                             break;
> > > +
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_header);
> > > +
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size) {
> > > +     u32 i;
> > > +     int ret;
> > > +
> > > +     for (i = 0; i < buff_size; i++) {
> > > +             ret = cdns_mhdp_mailbox_read(base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             buff[i] = ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_recv_data);
> > > +
> > > +int cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8
> > 
> > module_id,
> > 
> > > +                        u8 opcode, u16 size, u8 *message) {
> > > +     u8 header[4];
> > > +     int ret, i;
> > > +
> > > +     header[0] = opcode;
> > > +     header[1] = module_id;
> > > +     put_unaligned_be16(size, header + 2);
> > > +
> > > +     for (i = 0; i < sizeof(header); i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, header[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     for (i = 0; i < size; i++) {
> > > +             ret = cdns_mhdp_mailbox_write(base, message[i]);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_mailbox_send);
> > > +
> > > +/* General helper functions */
> > > +int cdns_mhdp_reg_read(struct cdns_mhdp_base *base, u32 addr, u32
> > > +*value) {
> > > +     u8 msg[4], resp[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_READ,
> > > +                                  sizeof(msg), msg);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_GENERAL,
> > 
> > > +
> > 
> > GENERAL_REGISTER_READ,
> > 
> > > +                                         sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, resp, sizeof(resp));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     /* Returned address value should be the same as requested */
> > > +     if (memcmp(msg, resp, sizeof(msg))) {
> > > +             ret = -EINVAL;
> > > +             goto out;
> > > +     }
> > > +
> > > +     *value = get_unaligned_be32(resp + 4);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +     if (ret) {
> > > +             dev_err(base->dev, "Failed to read register\n");
> > > +             *value = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_read);
> > > +
> > > +int cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32
> > > +val) {
> > > +     u8 msg[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be32(addr, msg);
> > > +     put_unaligned_be32(val, msg + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_GENERAL,
> > > +                                  GENERAL_REGISTER_WRITE,
> > > +                                  sizeof(msg), msg);
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_reg_write);
> > > +
> > > +/* DPTX helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val) {
> > > +     u8 msg[6];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, msg);
> > > +     put_unaligned_be32(val, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_REGISTER,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write);
> > > +
> > > +int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16 addr,
> > > +                            u8 start_bit, u8 bits_no, u32 val) {
> > > +     u8 field[8];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(addr, field);
> > > +     field[2] = start_bit;
> > > +     field[3] = bits_no;
> > > +     put_unaligned_be32(val, field + 4);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_FIELD,
> > 
> > sizeof(field),
> > field);
> > 
> > > +
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dp_reg_write_bit);
> > > +
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len) {
> > > +     u8 msg[5], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(len, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_READ_DPCD, sizeof(msg),
> > 
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_READ_DPCD,
> > > +                                         sizeof(reg) + len);
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, data, len);
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_read);
> > > +
> > > +int cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value) {
> > > +     u8 msg[6], reg[5];
> > > +     int ret;
> > > +
> > > +     put_unaligned_be16(1, msg);
> > > +     put_unaligned_be24(addr, msg + 2);
> > > +     msg[5] = value;
> > > +
> > > +     mutex_lock(base->mbox_mutex);
> > > +
> > > +     ret = cdns_mhdp_mailbox_send(base, MB_MODULE_ID_DP_TX,
> > > +                                  DPTX_WRITE_DPCD,
> > 
> > sizeof(msg),
> > msg);
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_header(base,
> > 
> > MB_MODULE_ID_DP_TX,
> > 
> > > +                                         DPTX_WRITE_DPCD,
> > 
> > sizeof(reg));
> > 
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     ret = cdns_mhdp_mailbox_recv_data(base, reg, sizeof(reg));
> > > +     if (ret)
> > > +             goto out;
> > > +
> > > +     if (addr != get_unaligned_be24(reg + 2))
> > > +             ret = -EINVAL;
> > > +
> > > +out:
> > > +     mutex_unlock(base->mbox_mutex);
> > > +
> > > +     if (ret)
> > > +             dev_err(base->dev, "dpcd write failed: %d\n", ret);
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(cdns_mhdp_dpcd_write);
> > > +
> > > +MODULE_DESCRIPTION("Cadence MHDP Helper driver");
> > > +MODULE_AUTHOR("Sandor Yu <Sandor.yu@nxp.com>");
> > > +MODULE_LICENSE("GPL");
> > > [...]
> > > diff --git a/include/drm/bridge/cdns-mhdp-helper.h
> > > b/include/drm/bridge/cdns-mhdp-helper.h new file mode 100644 index
> > > 0000000000000..b89db9e842266
> > > --- /dev/null
> > > +++ b/include/drm/bridge/cdns-mhdp-helper.h
> > > @@ -0,0 +1,96 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 NXP Semiconductor, Inc.
> > > + */
> > > +#ifndef __CDNS_MHDP_HELPER_H__
> > > +#define __CDNS_MHDP_HELPER_H__
> > > +
> > > +#include <asm/unaligned.h>
> > > +#include <linux/iopoll.h>
> > > +
> > > +/* mailbox regs offset */
> > > +#define CDNS_MAILBOX_FULL                    0x00008
> > > +#define CDNS_MAILBOX_EMPTY                   0x0000c
> > > +#define CDNS_MAILBOX_TX_DATA                 0x00010
> > > +#define CDNS_MAILBOX_RX_DATA                 0x00014
> > > +
> > > +#define MAILBOX_RETRY_US                     1000
> > > +#define MAILBOX_TIMEOUT_US                   2000000
> > > +
> > > +/* Module ID Code */
> > > +#define MB_MODULE_ID_DP_TX                   0x01
> > > +#define MB_MODULE_ID_HDMI_TX                 0x03
> > > +#define MB_MODULE_ID_HDCP_TX                 0x07
> > > +#define MB_MODULE_ID_HDCP_RX                 0x08
> > > +#define MB_MODULE_ID_HDCP_GENERAL            0x09
> > > +#define MB_MODULE_ID_GENERAL                 0x0A
> > > +
> > > +/* General Commands */
> > > +#define GENERAL_MAIN_CONTROL                 0x01
> > > +#define GENERAL_TEST_ECHO                    0x02
> > > +#define GENERAL_BUS_SETTINGS                 0x03
> > > +#define GENERAL_TEST_ACCESS                  0x04
> > > +#define GENERAL_REGISTER_WRITE                       0x05
> > > +#define GENERAL_WRITE_FIELD                  0x06
> > > +#define GENERAL_REGISTER_READ                        0x07
> > > +#define GENERAL_GET_HPD_STATE                        0x11
> > > +
> > > +/* DPTX Commands */
> > > +#define DPTX_SET_POWER_MNG                   0x00
> > > +#define DPTX_SET_HOST_CAPABILITIES           0x01
> > > +#define DPTX_GET_EDID                                0x02
> > > +#define DPTX_READ_DPCD                               0x03
> > > +#define DPTX_WRITE_DPCD                              0x04
> > > +#define DPTX_ENABLE_EVENT                    0x05
> > > +#define DPTX_WRITE_REGISTER                  0x06
> > > +#define DPTX_READ_REGISTER                   0x07
> > > +#define DPTX_WRITE_FIELD                     0x08
> > > +#define DPTX_TRAINING_CONTROL                        0x09
> > > +#define DPTX_READ_EVENT                              0x0a
> > > +#define DPTX_READ_LINK_STAT                  0x0b
> > > +#define DPTX_SET_VIDEO                               0x0c
> > > +#define DPTX_SET_AUDIO                               0x0d
> > > +#define DPTX_GET_LAST_AUX_STAUS                      0x0e
> > > +#define DPTX_SET_LINK_BREAK_POINT            0x0f
> > > +#define DPTX_FORCE_LANES                     0x10
> > > +#define DPTX_HPD_STATE                               0x11
> > > +#define DPTX_ADJUST_LT                               0x12
> > > +
> > > +/* HDMI TX Commands */
> > > +#define HDMI_TX_READ                         0x00
> > > +#define HDMI_TX_WRITE                                0x01
> > > +#define HDMI_TX_UPDATE_READ                  0x02
> > > +#define HDMI_TX_EDID                         0x03
> > > +#define HDMI_TX_EVENTS                               0x04
> > > +#define HDMI_TX_HPD_STATUS                   0x05
> > > +
> > > +struct cdns_mhdp_base {
> > > +     struct device *dev;
> > > +     void __iomem *regs;
> > > +     /* protect mailbox communications with the firmware */
> > > +     struct mutex *mbox_mutex;
> > > +};
> > > +
> > > +/* Mailbox helper functions */
> > > +int cdns_mhdp_mailbox_read(struct cdns_mhdp_base *base); int
> > > +cdns_mhdp_mailbox_write(struct cdns_mhdp_base *base, u8 val);
> > 
> > You can remove these two declarations.
> 
> cdns_mhdp_mailbox_read will be removed. Thanks!
> 
> B.R
> Sandor
> 
> > Best regards,
> > Alexander
> > 
> > Best regards,
> > 
> > > +int cdns_mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> > > +                               u8 module_id, u8 opcode, u16
> > 
> > req_size);
> > 
> > > +int cdns_mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> > > +                             u8 *buff, u16 buff_size); int
> > > +cdns_mhdp_mailbox_send(struct cdns_mhdp_base *base, u8 module_id,
> > > +                        u8 opcode, u16 size, u8 *message);
> > > +
> > > +/* General commands helper functions */ int cdns_mhdp_reg_read(struct
> > > +cdns_mhdp_base *base, u32 addr, u32 *value); int
> > > +cdns_mhdp_reg_write(struct cdns_mhdp_base *base, u32 addr, u32 val);
> > > +
> > > +/* DPTX commands helper functions */
> > > +int cdns_mhdp_dp_reg_write(struct cdns_mhdp_base *base, u16 addr,
> > 
> > u32
> > 
> > > +val); int cdns_mhdp_dp_reg_write_bit(struct cdns_mhdp_base *base, u16
> > > +addr, +
> > > 
> > >       u8 start_bit, u8 bits_no, u32 val);
> > > 
> > > +int cdns_mhdp_dpcd_read(struct cdns_mhdp_base *base,
> > > +                     u32 addr, u8 *data, u16 len); int
> > > +cdns_mhdp_dpcd_write(struct cdns_mhdp_base *base, u32 addr, u8
> > > +value);
> > > +
> > > +#endif /* __CDNS_MHDP_HELPER_H__ */
> > 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



  reply	other threads:[~2023-10-16  8:30 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-13  3:24 [PATCH v10 0/7] Initial support Cadence MHDP8501(HDMI/DP) for i.MX8MQ Sandor Yu
2023-10-13  3:24 ` Sandor Yu
2023-10-13  3:24 ` Sandor Yu
2023-10-13  3:24 ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 1/7] drm: bridge: Cadence: Creat mhdp helper driver Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  7:28   ` Alexander Stein
2023-10-13  7:28     ` Alexander Stein
2023-10-13  7:28     ` Alexander Stein
2023-10-13  7:28     ` Alexander Stein
2023-10-16  3:05     ` [EXT] " Sandor Yu
2023-10-16  3:05       ` Sandor Yu
2023-10-16  3:05       ` Sandor Yu
2023-10-16  3:05       ` Sandor Yu
2023-10-16  8:30       ` Alexander Stein [this message]
2023-10-16  8:30         ` Alexander Stein
2023-10-16  8:30         ` Alexander Stein
2023-10-16  8:30         ` Alexander Stein
2023-10-16  9:19         ` [EXT] " Sandor Yu
2023-10-16  9:19           ` Sandor Yu
2023-10-16  9:19           ` Sandor Yu
2023-10-16  9:19           ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 2/7] phy: Add HDMI configuration options Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 3/7] dt-bindings: display: bridge: Add Cadence MHDP8501 Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 4/7] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  7:28   ` Alexander Stein
2023-10-13  7:28     ` Alexander Stein
2023-10-13  7:28     ` Alexander Stein
2023-10-13  7:28     ` Alexander Stein
2023-10-16  3:05     ` [EXT] " Sandor Yu
2023-10-16  3:05       ` Sandor Yu
2023-10-16  3:05       ` Sandor Yu
2023-10-16  3:05       ` Sandor Yu
2023-10-16 11:56   ` Krzysztof Kozlowski
2023-10-16 11:56     ` Krzysztof Kozlowski
2023-10-16 11:56     ` Krzysztof Kozlowski
2023-10-16 11:56     ` Krzysztof Kozlowski
2023-10-17  3:17     ` [EXT] " Sandor Yu
2023-10-17  3:17       ` Sandor Yu
2023-10-17  3:17       ` Sandor Yu
2023-10-17  3:17       ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 5/7] dt-bindings: phy: Add Freescale iMX8MQ DP and HDMI PHY Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 6/7] phy: freescale: Add DisplayPort PHY driver for i.MX8MQ Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24 ` [PATCH v10 7/7] phy: freescale: Add HDMI " Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu
2023-10-13  3:24   ` Sandor Yu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3258050.aeNJFYEL58@steina-w \
    --to=alexander.stein@ew.tq-group.com \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=airlied@gmail.com \
    --cc=andrzej.hajda@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=festevam@gmail.com \
    --cc=jernej.skrabec@gmail.com \
    --cc=jonas@kwiboo.se \
    --cc=kernel@pengutronix.de \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-phy@lists.infradead.org \
    --cc=neil.armstrong@linaro.org \
    --cc=oliver.brown@nxp.com \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=sam@ravnborg.org \
    --cc=sandor.yu@nxp.com \
    --cc=shawnguo@kernel.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.