On Mon 19 Aug 2024 at 18:22, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
> On 30/07/2024 14:50, Jerome Brunet wrote:
>> The Amlogic mixes direct register access and regmap ones, with several
>> custom helpers. Using a single API makes rework and maintenance easier.
>> Convert the Amlogic phy driver to regmap and use it to have more
>> consistent
>> access to the registers in the driver, with less custom helpers. Add
>> register bit definitions when missing.
>> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>> ---
>> drivers/gpu/drm/meson/meson_dw_hdmi.c | 475 ++++++++++++--------------
>> drivers/gpu/drm/meson/meson_dw_hdmi.h | 49 +--
>> 2 files changed, 239 insertions(+), 285 deletions(-)
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index 47aa3e184e98..7c39e5c99043 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -90,16 +90,25 @@
>> * - CEC Management
>> */
>> -/* TOP Block Communication Channel */
>> -#define HDMITX_TOP_ADDR_REG 0x0
>> -#define HDMITX_TOP_DATA_REG 0x4
>> -#define HDMITX_TOP_CTRL_REG 0x8
>> -#define HDMITX_TOP_G12A_OFFSET 0x8000
>> +/* Indirect channel definition for GX */
>> +#define HDMITX_TOP_REGS 0x0
>> +#define HDMITX_DWC_REGS 0x10
>> +
>> +#define GX_ADDR_OFFSET 0x0
>> +#define GX_DATA_OFFSET 0x4
>> +#define GX_CTRL_OFFSET 0x8
>
> I don't see the point renaming thos defines
It makes clear that indirect addressing offset are for GX SoC only,
which the original define did not do
>
>> +#define GX_CTRL_APB3_ERRFAIL BIT(15)
>> -/* Controller Communication Channel */
>> -#define HDMITX_DWC_ADDR_REG 0x10
>> -#define HDMITX_DWC_DATA_REG 0x14
>> -#define HDMITX_DWC_CTRL_REG 0x18
>> +/*
>> + * NOTE: G12 use direct addressing:
>> + * Ideally it should receive one memory region for each of the top
>> + * and dwc register regions but fixing this would require to change
>> + * the DT bindings. Doing so is a pain. Keep the region as it and work
>> + * around the problem, at least for now.
>> + * Future supported SoCs should properly describe the regions in the
>> + * DT bindings instead of using this trick.
>
> well I disagree here, there's a single memory region for the HDMITX module,
> and the DWC region is controlled by the TOP registers, so the DWC region
> _cannot_ be accessed without correctly configuring the TOP registers.
>
> So I disagree strongly with this comment.
Even if one cannot be accessed without configuring the other, it does
not make it a single region.
The regions do not even use the same register value width.
That's a clear indication they are separate.
Anyway, I'll remove the comment
>
>> + */
>> +#define HDMITX_TOP_G12A_OFFSET 0x8000
>> /* HHI Registers */
>> #define HHI_MEM_PD_REG0 0x100 /* 0x40 */
>> @@ -108,28 +117,59 @@
>> #define HHI_HDMI_PHY_CNTL1 0x3a4 /* 0xe9 */
>> #define PHY_CNTL1_INIT 0x03900000
>> #define PHY_INVERT BIT(17)
>> +#define PHY_FIFOS GENMASK(3, 2)
>> +#define PHY_CLOCK_EN BIT(1)
>> +#define PHY_SOFT_RST BIT(0)
>
> Please move those changes before or after this patch
>
>> #define HHI_HDMI_PHY_CNTL2 0x3a8 /* 0xea */
>> #define HHI_HDMI_PHY_CNTL3 0x3ac /* 0xeb */
>> #define HHI_HDMI_PHY_CNTL4 0x3b0 /* 0xec */
>> #define HHI_HDMI_PHY_CNTL5 0x3b4 /* 0xed */
>> -static DEFINE_SPINLOCK(reg_lock);
>> -
>> -struct meson_dw_hdmi;
>> -
>> struct meson_dw_hdmi_data {
>> - unsigned int (*top_read)(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr);
>> - void (*top_write)(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr, unsigned int data);
>> - unsigned int (*dwc_read)(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr);
>> - void (*dwc_write)(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr, unsigned int data);
>> + int (*reg_init)(struct device *dev);
>> u32 cntl0_init;
>> u32 cntl1_init;
>> };
>> +static int hdmi_tx_indirect_reg_read(void *context,
>> + unsigned int reg,
>> + unsigned int *result)
>> +{
>> + void __iomem *base = context;
>> +
>> + /* Must write the read address twice ... */
>> + writel(reg, base + GX_ADDR_OFFSET);
>> + writel(reg, base + GX_ADDR_OFFSET);
>> +
>> + /* ... and read the data twice as well */
>> + *result = readl(base + GX_DATA_OFFSET);
>> + *result = readl(base + GX_DATA_OFFSET);
>
> Why did you change the comments ?
>
>> +
>> + return 0;
>> +}
>> +
>> +static int hdmi_tx_indirect_reg_write(void *context,
>> + unsigned int reg,
>> + unsigned int val)
>> +{
>> + void __iomem *base = context;
>> +
>> + /* Must write the read address twice ... */
>> + writel(reg, base + GX_ADDR_OFFSET);
>> + writel(reg, base + GX_ADDR_OFFSET);
>> +
>> + /* ... but write the data only once */
>> + writel(val, base + GX_DATA_OFFSET);
>
> Ditto ? were they wrong ?
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct regmap_bus hdmi_tx_indirect_mmio = {
>> + .fast_io = true,
>> + .reg_read = hdmi_tx_indirect_reg_read,
>> + .reg_write = hdmi_tx_indirect_reg_write,
>> +};
>> +
>> struct meson_dw_hdmi {
>> struct dw_hdmi_plat_data dw_plat_data;
>> struct meson_drm *priv;
>> @@ -139,9 +179,10 @@ struct meson_dw_hdmi {
>> struct reset_control *hdmitx_apb;
>> struct reset_control *hdmitx_ctrl;
>> struct reset_control *hdmitx_phy;
>> - u32 irq_stat;
>> + unsigned int irq_stat;
>> struct dw_hdmi *hdmi;
>> struct drm_bridge *bridge;
>> + struct regmap *top;
>
> The name could be better, like top_regs or top_regmap
That's not really consistent with priv->hhi, is it ?
>
>> };
>> static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi
>> *dw_hdmi,
>> @@ -150,136 +191,6 @@ static inline int dw_hdmi_is_compatible(struct meson_dw_hdmi *dw_hdmi,
>> return of_device_is_compatible(dw_hdmi->dev->of_node, compat);
>> }
>> -/* PHY (via TOP bridge) and Controller dedicated register interface */
>> -
>> -static unsigned int dw_hdmi_top_read(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr)
>> -{
>> - unsigned long flags;
>> - unsigned int data;
>> -
>> - spin_lock_irqsave(®_lock, flags);
>> -
>> - /* ADDR must be written twice */
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
>> -
>> - /* Read needs a second DATA read */
>> - data = readl(dw_hdmi->hdmitx + HDMITX_TOP_DATA_REG);
>> - data = readl(dw_hdmi->hdmitx + HDMITX_TOP_DATA_REG);
>> -
>> - spin_unlock_irqrestore(®_lock, flags);
>> -
>> - return data;
>> -}
>> -
>> -static unsigned int dw_hdmi_g12a_top_read(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr)
>> -{
>> - return readl(dw_hdmi->hdmitx + HDMITX_TOP_G12A_OFFSET + (addr << 2));
>> -}
>> -
>> -static inline void dw_hdmi_top_write(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr, unsigned int data)
>> -{
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(®_lock, flags);
>> -
>> - /* ADDR must be written twice */
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_TOP_ADDR_REG);
>> -
>> - /* Write needs single DATA write */
>> - writel(data, dw_hdmi->hdmitx + HDMITX_TOP_DATA_REG);
>> -
>> - spin_unlock_irqrestore(®_lock, flags);
>> -}
>> -
>> -static inline void dw_hdmi_g12a_top_write(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr, unsigned int data)
>> -{
>> - writel(data, dw_hdmi->hdmitx + HDMITX_TOP_G12A_OFFSET + (addr << 2));
>> -}
>> -
>> -/* Helper to change specific bits in PHY registers */
>> -static inline void dw_hdmi_top_write_bits(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr,
>> - unsigned int mask,
>> - unsigned int val)
>> -{
>> - unsigned int data = dw_hdmi->data->top_read(dw_hdmi, addr);
>> -
>> - data &= ~mask;
>> - data |= val;
>> -
>> - dw_hdmi->data->top_write(dw_hdmi, addr, data);
>> -}
>> -
>> -static unsigned int dw_hdmi_dwc_read(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr)
>> -{
>> - unsigned long flags;
>> - unsigned int data;
>> -
>> - spin_lock_irqsave(®_lock, flags);
>> -
>> - /* ADDR must be written twice */
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
>> -
>> - /* Read needs a second DATA read */
>> - data = readl(dw_hdmi->hdmitx + HDMITX_DWC_DATA_REG);
>> - data = readl(dw_hdmi->hdmitx + HDMITX_DWC_DATA_REG);
>> -
>> - spin_unlock_irqrestore(®_lock, flags);
>> -
>> - return data;
>> -}
>> -
>> -static unsigned int dw_hdmi_g12a_dwc_read(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr)
>> -{
>> - return readb(dw_hdmi->hdmitx + addr);
>> -}
>> -
>> -static inline void dw_hdmi_dwc_write(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr, unsigned int data)
>> -{
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(®_lock, flags);
>> -
>> - /* ADDR must be written twice */
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
>> - writel(addr & 0xffff, dw_hdmi->hdmitx + HDMITX_DWC_ADDR_REG);
>> -
>> - /* Write needs single DATA write */
>> - writel(data, dw_hdmi->hdmitx + HDMITX_DWC_DATA_REG);
>> -
>> - spin_unlock_irqrestore(®_lock, flags);
>> -}
>> -
>> -static inline void dw_hdmi_g12a_dwc_write(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr, unsigned int data)
>> -{
>> - writeb(data, dw_hdmi->hdmitx + addr);
>> -}
>> -
>> -/* Helper to change specific bits in controller registers */
>> -static inline void dw_hdmi_dwc_write_bits(struct meson_dw_hdmi *dw_hdmi,
>> - unsigned int addr,
>> - unsigned int mask,
>> - unsigned int val)
>> -{
>> - unsigned int data = dw_hdmi->data->dwc_read(dw_hdmi, addr);
>> -
>> - data &= ~mask;
>> - data |= val;
>> -
>> - dw_hdmi->data->dwc_write(dw_hdmi, addr, data);
>> -}
>> -
>> /* Bridge */
>> /* Setup PHY bandwidth modes */
>> @@ -353,13 +264,15 @@ static inline void meson_dw_hdmi_phy_reset(struct meson_dw_hdmi *dw_hdmi)
>> struct meson_drm *priv = dw_hdmi->priv;
>> /* Enable and software reset */
>> - regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0xf);
>> -
>> + regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
>> + PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST,
>> + PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST);
>> mdelay(2);
>> /* Enable and unreset */
>> - regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0xe);
>> -
>> + regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
>> + PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST,
>> + PHY_FIFOS | PHY_CLOCK_EN);
>> mdelay(2);
>> }
>> @@ -382,27 +295,30 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi,
>> void *data,
>> /* TMDS pattern setup */
>> if (mode->clock > 340000 && !mode_is_420) {
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
>> - 0);
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
>> - 0x03ff03ff);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_01,
>> + 0);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_23,
>> + 0x03ff03ff);
>> } else {
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
>> - 0x001f001f);
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_23,
>> - 0x001f001f);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_01,
>> + 0x001f001f);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_23,
>> + 0x001f001f);
>> }
>> /* Load TMDS pattern */
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_CNTL, 0x1);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_CNTL,
>> + TOP_TDMS_CLK_PTTN_LOAD);
>> msleep(20);
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_CNTL, 0x2);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_TMDS_CLK_PTTN_CNTL,
>> + TOP_TDMS_CLK_PTTN_SHFT);
>> /* Setup PHY parameters */
>> meson_hdmi_phy_setup_mode(dw_hdmi, mode, mode_is_420);
>> /* Disable clock, fifo, fifo_wr */
>> - regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0);
>> + regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
>> + PHY_FIFOS | PHY_CLOCK_EN | PHY_SOFT_RST, 0);
>> dw_hdmi_set_high_tmds_clock_ratio(hdmi, display);
>> @@ -433,8 +349,11 @@ static enum drm_connector_status
>> dw_hdmi_read_hpd(struct dw_hdmi *hdmi,
>> void *data)
>> {
>> struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
>> + unsigned int stat;
>> - return !!dw_hdmi->data->top_read(dw_hdmi, HDMITX_TOP_STAT0) ?
>> + regmap_read(dw_hdmi->top, HDMITX_TOP_STAT0, &stat);
>> +
>> + return !!stat ?
>> connector_status_connected : connector_status_disconnected;
>> }
>> @@ -444,17 +363,18 @@ static void dw_hdmi_setup_hpd(struct dw_hdmi
>> *hdmi,
>> struct meson_dw_hdmi *dw_hdmi = (struct meson_dw_hdmi *)data;
>> /* Setup HPD Filter */
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_HPD_FILTER,
>> - (0xa << 12) | 0xa0);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_HPD_FILTER,
>> + FIELD_PREP(TOP_HPD_GLITCH_WIDTH, 10) |
>> + FIELD_PREP(TOP_HPD_VALID_WIDTH, 160));
>> /* Clear interrupts */
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_INTR_STAT_CLR,
>> - HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_INTR_STAT_CLR,
>> + TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL);
>> /* Unmask interrupts */
>> - dw_hdmi_top_write_bits(dw_hdmi, HDMITX_TOP_INTR_MASKN,
>> - HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL,
>> - HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL);
>> + regmap_update_bits(dw_hdmi->top, HDMITX_TOP_INTR_MASKN,
>> + TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL,
>> + TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL);
>> }
>> static const struct dw_hdmi_phy_ops meson_dw_hdmi_phy_ops = {
>> @@ -467,23 +387,22 @@ static const struct dw_hdmi_phy_ops meson_dw_hdmi_phy_ops = {
>> static irqreturn_t dw_hdmi_top_irq(int irq, void *dev_id)
>> {
>> struct meson_dw_hdmi *dw_hdmi = dev_id;
>> - u32 stat;
>> + unsigned int stat;
>> - stat = dw_hdmi->data->top_read(dw_hdmi, HDMITX_TOP_INTR_STAT);
>> - dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_INTR_STAT_CLR, stat);
>> + regmap_read(dw_hdmi->top, HDMITX_TOP_INTR_STAT, &stat);
>> + regmap_write(dw_hdmi->top, HDMITX_TOP_INTR_STAT_CLR, stat);
>> /* HPD Events, handle in the threaded interrupt handler */
>> - if (stat & (HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL)) {
>> + if (stat & (TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL)) {
>> dw_hdmi->irq_stat = stat;
>> return IRQ_WAKE_THREAD;
>> }
>> /* HDMI Controller Interrupt */
>> - if (stat & 1)
>> + if (stat & TOP_INTR_CORE)
>> return IRQ_NONE;
>> /* TOFIX Handle HDCP Interrupts */
>> -
>> return IRQ_HANDLED;
>> }
>> @@ -494,10 +413,10 @@ static irqreturn_t dw_hdmi_top_thread_irq(int
>> irq, void *dev_id)
>> u32 stat = dw_hdmi->irq_stat;
>> /* HPD Events */
>> - if (stat & (HDMITX_TOP_INTR_HPD_RISE | HDMITX_TOP_INTR_HPD_FALL)) {
>> + if (stat & (TOP_INTR_HPD_RISE | TOP_INTR_HPD_FALL)) {
>> bool hpd_connected = false;
>> - if (stat & HDMITX_TOP_INTR_HPD_RISE)
>> + if (stat & TOP_INTR_HPD_RISE)
>> hpd_connected = true;
>> dw_hdmi_setup_rx_sense(dw_hdmi->hdmi, hpd_connected,
>> @@ -512,63 +431,25 @@ static irqreturn_t dw_hdmi_top_thread_irq(int irq, void *dev_id)
>> return IRQ_HANDLED;
>> }
>> -/* DW HDMI Regmap */
>> -
>> -static int meson_dw_hdmi_reg_read(void *context, unsigned int reg,
>> - unsigned int *result)
>> -{
>> - struct meson_dw_hdmi *dw_hdmi = context;
>> -
>> - *result = dw_hdmi->data->dwc_read(dw_hdmi, reg);
>> -
>> - return 0;
>> -
>> -}
>> -
>> -static int meson_dw_hdmi_reg_write(void *context, unsigned int reg,
>> - unsigned int val)
>> -{
>> - struct meson_dw_hdmi *dw_hdmi = context;
>> -
>> - dw_hdmi->data->dwc_write(dw_hdmi, reg, val);
>> -
>> - return 0;
>> -}
>> -
>> -static const struct regmap_config meson_dw_hdmi_regmap_config = {
>> - .reg_bits = 32,
>> - .val_bits = 8,
>> - .reg_read = meson_dw_hdmi_reg_read,
>> - .reg_write = meson_dw_hdmi_reg_write,
>> - .max_register = 0x10000,
>> - .fast_io = true,
>> -};
>> -
>> -static const struct meson_dw_hdmi_data meson_dw_hdmi_gxbb_data = {
>> - .top_read = dw_hdmi_top_read,
>> - .top_write = dw_hdmi_top_write,
>> - .dwc_read = dw_hdmi_dwc_read,
>> - .dwc_write = dw_hdmi_dwc_write,
>> - .cntl0_init = 0x0,
>> - .cntl1_init = PHY_CNTL1_INIT | PHY_INVERT,
>> +static const struct regmap_config top_gx_regmap_cfg = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .reg_shift = -2,
>> + .val_bits = 32,
>> + .max_register = 0x40,
>> };
>> -static const struct meson_dw_hdmi_data meson_dw_hdmi_gxl_data = {
>> - .top_read = dw_hdmi_top_read,
>> - .top_write = dw_hdmi_top_write,
>> - .dwc_read = dw_hdmi_dwc_read,
>> - .dwc_write = dw_hdmi_dwc_write,
>> - .cntl0_init = 0x0,
>> - .cntl1_init = PHY_CNTL1_INIT,
>> +static const struct regmap_config top_g12_regmap_cfg = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> + .max_register = 0x40,
>> };
>> -static const struct meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
>> - .top_read = dw_hdmi_g12a_top_read,
>> - .top_write = dw_hdmi_g12a_top_write,
>> - .dwc_read = dw_hdmi_g12a_dwc_read,
>> - .dwc_write = dw_hdmi_g12a_dwc_write,
>> - .cntl0_init = 0x000b4242, /* Bandgap */
>> - .cntl1_init = PHY_CNTL1_INIT,
>> +static const struct regmap_config dwc_regmap_cfg = {
>> + .reg_bits = 32,
>> + .val_bits = 8,
>> + .max_register = 0x8000,
>> };
>> static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
>> @@ -581,41 +462,107 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
>> /* Bring HDMITX MEM output of power down */
>> regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0);
>> - /* Enable APB3 fail on error */
>> - if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
>> - writel_bits_relaxed(BIT(15), BIT(15),
>> - meson_dw_hdmi->hdmitx + HDMITX_TOP_CTRL_REG);
>> - writel_bits_relaxed(BIT(15), BIT(15),
>> - meson_dw_hdmi->hdmitx + HDMITX_DWC_CTRL_REG);
>> - }
>> -
>> /* Bring out of reset */
>> - meson_dw_hdmi->data->top_write(meson_dw_hdmi,
>> - HDMITX_TOP_SW_RESET, 0);
>> -
>> + regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
>> msleep(20);
>> - meson_dw_hdmi->data->top_write(meson_dw_hdmi,
>> - HDMITX_TOP_CLK_CNTL, 0xff);
>> + /* Enable clocks */
>> + regmap_write(meson_dw_hdmi->top, HDMITX_TOP_CLK_CNTL,
>> + TOP_CLK_EN);
>> /* Enable normal output to PHY */
>> - meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>> + regmap_write(meson_dw_hdmi->top, HDMITX_TOP_BIST_CNTL,
>> + TOP_BIST_TMDS_EN);
>> /* Setup PHY */
>> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL1, meson_dw_hdmi->data->cntl1_init);
>> - regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, meson_dw_hdmi->data->cntl0_init);
>> + regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL1,
>> + meson_dw_hdmi->data->cntl1_init);
>> + regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0,
>> + meson_dw_hdmi->data->cntl0_init);
>> /* Enable HDMI-TX Interrupt */
>> - meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_INTR_STAT_CLR,
>> - HDMITX_TOP_INTR_CORE);
>> + regmap_write(meson_dw_hdmi->top, HDMITX_TOP_INTR_STAT_CLR,
>> + GENMASK(31, 0));
>> + regmap_write(meson_dw_hdmi->top, HDMITX_TOP_INTR_MASKN,
>> + TOP_INTR_CORE);
>> +}
>> +
>> +static int meson_dw_init_regmap_gx(struct device *dev)
>> +{
>> + struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
>> + struct regmap *map;
>> - meson_dw_hdmi->data->top_write(meson_dw_hdmi,
>> HDMITX_TOP_INTR_MASKN,
>> - HDMITX_TOP_INTR_CORE);
>> + /* Register TOP glue zone */
>> + writel_bits_relaxed(GX_CTRL_APB3_ERRFAIL, GX_CTRL_APB3_ERRFAIL,
>> + meson_dw_hdmi->hdmitx + HDMITX_TOP_REGS + GX_CTRL_OFFSET);
>> + map = devm_regmap_init(dev, &hdmi_tx_indirect_mmio,
>> + meson_dw_hdmi->hdmitx + HDMITX_TOP_REGS,
>> + &top_gx_regmap_cfg);
>> + if (IS_ERR(map))
>> + return dev_err_probe(dev, PTR_ERR(map), "failed to init top regmap\n");
>> +
>> + meson_dw_hdmi->top = map;
>> +
>> + /* Register DWC zone */
>> + writel_bits_relaxed(GX_CTRL_APB3_ERRFAIL, GX_CTRL_APB3_ERRFAIL,
>> + meson_dw_hdmi->hdmitx + HDMITX_DWC_REGS + GX_CTRL_OFFSET);
>> +
>> + map = devm_regmap_init(dev, &hdmi_tx_indirect_mmio,
>> + meson_dw_hdmi->hdmitx + HDMITX_DWC_REGS,
>> + &dwc_regmap_cfg);
>> + if (IS_ERR(map))
>> + return dev_err_probe(dev, PTR_ERR(map), "failed to init dwc regmap\n");
>> +
>> + meson_dw_hdmi->dw_plat_data.regm = map;
>> +
>> + return 0;
>> +}
>> +
>> +static int meson_dw_init_regmap_g12(struct device *dev)
>> +{
>> + struct meson_dw_hdmi *meson_dw_hdmi = dev_get_drvdata(dev);
>> + struct regmap *map;
>> +
>> + /* Register TOP glue zone with the offset */
>> + map = devm_regmap_init_mmio(dev, meson_dw_hdmi->hdmitx + HDMITX_TOP_G12A_OFFSET,
>> + &top_g12_regmap_cfg);
>> + if (IS_ERR(map))
>> + dev_err_probe(dev, PTR_ERR(map), "failed to init top regmap\n");
>> +
>> + meson_dw_hdmi->top = map;
>> +
>> + /* Register DWC zone */
>> + map = devm_regmap_init_mmio(dev, meson_dw_hdmi->hdmitx,
>> + &dwc_regmap_cfg);
>> + if (IS_ERR(map))
>> + dev_err_probe(dev, PTR_ERR(map), "failed to init dwc regmap\n");
>> +
>> + meson_dw_hdmi->dw_plat_data.regm = map;
>> +
>> + return 0;
>> }
>> +static const struct meson_dw_hdmi_data meson_dw_hdmi_gxbb_data = {
>> + .reg_init = meson_dw_init_regmap_gx,
>> + .cntl0_init = 0x0,
>> + .cntl1_init = PHY_CNTL1_INIT | PHY_INVERT,
>> +};
>> +
>> +static const struct meson_dw_hdmi_data meson_dw_hdmi_gxl_data = {
>> + .reg_init = meson_dw_init_regmap_gx,
>> + .cntl0_init = 0x0,
>> + .cntl1_init = PHY_CNTL1_INIT,
>> +};
>> +
>> +static const struct meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
>> + .reg_init = meson_dw_init_regmap_g12,
>> + .cntl0_init = 0x000b4242, /* Bandgap */
>> + .cntl1_init = PHY_CNTL1_INIT,
>> +};
>> +
>> static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>> - void *data)
>> + void *data)
>> {
>> struct platform_device *pdev = to_platform_device(dev);
>> const struct meson_dw_hdmi_data *match;
>> @@ -640,6 +587,8 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>> if (!meson_dw_hdmi)
>> return -ENOMEM;
>> + platform_set_drvdata(pdev, meson_dw_hdmi);
>> +
>> meson_dw_hdmi->priv = priv;
>> meson_dw_hdmi->dev = dev;
>> meson_dw_hdmi->data = match;
>> @@ -682,10 +631,9 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>> if (ret)
>> return dev_err_probe(dev, ret, "Failed to enable all clocks\n");
>> - dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi,
>> - &meson_dw_hdmi_regmap_config);
>> - if (IS_ERR(dw_plat_data->regm))
>> - return PTR_ERR(dw_plat_data->regm);
>> + ret = meson_dw_hdmi->data->reg_init(dev);
>> + if (ret)
>> + return ret;
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> @@ -717,8 +665,6 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
>> dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
>> dw_plat_data->use_drm_infoframe = true;
>> - platform_set_drvdata(pdev, meson_dw_hdmi);
>> -
>> meson_dw_hdmi->hdmi = dw_hdmi_probe(pdev, &meson_dw_hdmi->dw_plat_data);
>> if (IS_ERR(meson_dw_hdmi->hdmi))
>> return PTR_ERR(meson_dw_hdmi->hdmi);
>> @@ -751,8 +697,7 @@ static int __maybe_unused meson_dw_hdmi_pm_suspend(struct device *dev)
>> return 0;
>> /* FIXME: This actually bring top out reset on suspend, why ? */
>> - meson_dw_hdmi->data->top_write(meson_dw_hdmi,
>> - HDMITX_TOP_SW_RESET, 0);
>> + regmap_write(meson_dw_hdmi->top, HDMITX_TOP_SW_RESET, 0);
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.h b/drivers/gpu/drm/meson/meson_dw_hdmi.h
>> index 08e1c14e4ea0..3ab8c56d5fe1 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.h
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.h
>> @@ -28,6 +28,7 @@
>> * 0=Release from reset. Default 1.
>> */
>> #define HDMITX_TOP_SW_RESET (0x000)
>> +#define TOP_RST_EN GENMASK(4, 0)
>
> Well NAK, the registers are named HDMITX_TOP_XXXX in the datasheet,
> and TOP_XXXX defines could collide with other too generic defines,
> and in any case don't mix defines renaming with other changes.
>
> Just move the GENMASK in a new patch and leave the HDMITX_ prefix alone.
>
>> /*
>> * Bit 31 RW free_clk_en: 0=Enable clock gating for power saving; 1= Disable
>> @@ -45,7 +46,8 @@
>> * Bit 1 RW tmds_clk_en: 1=enable tmds_clk; 0=disable. Default 0.
>> * Bit 0 RW pixel_clk_en: 1=enable pixel_clk; 0=disable. Default 0.
>> */
>> -#define HDMITX_TOP_CLK_CNTL (0x001)
>> +#define HDMITX_TOP_CLK_CNTL (0x004)
>> +#define TOP_CLK_EN GENMASK(7, 0)
>> /*
>> * Bit 31:28 RW rxsense_glitch_width: starting from G12A
>> @@ -53,7 +55,9 @@
>> * Bit 11: 0 RW hpd_valid_width: filter out width <= M*1024. Default 0.
>> * Bit 15:12 RW hpd_glitch_width: filter out glitch <= N. Default 0.
>> */
>> -#define HDMITX_TOP_HPD_FILTER (0x002)
>> +#define HDMITX_TOP_HPD_FILTER (0x008)
>> +#define TOP_HPD_GLITCH_WIDTH GENMASK(15, 12)
>> +#define TOP_HPD_VALID_WIDTH GENMASK(11, 0)
>> /*
>> * intr_maskn: MASK_N, one bit per interrupt source.
>> @@ -67,7 +71,7 @@
>> * [ 1] hpd_rise_intr
>> * [ 0] core_intr
>> */
>> -#define HDMITX_TOP_INTR_MASKN (0x003)
>> +#define HDMITX_TOP_INTR_MASKN (0x00c)
>> /*
>> * Bit 30: 0 RW intr_stat: For each bit, write 1 to manually set the interrupt
>> @@ -80,7 +84,7 @@
>> * Bit 1 RW hpd_rise
>> * Bit 0 RW IP interrupt
>> */
>> -#define HDMITX_TOP_INTR_STAT (0x004)
>> +#define HDMITX_TOP_INTR_STAT (0x010)
>> /*
>> * [7] rxsense_fall starting from G12A
>> @@ -92,13 +96,12 @@
>> * [1] hpd_rise
>> * [0] core_intr_rise
>> */
>> -#define HDMITX_TOP_INTR_STAT_CLR (0x005)
>> -
>> -#define HDMITX_TOP_INTR_CORE BIT(0)
>> -#define HDMITX_TOP_INTR_HPD_RISE BIT(1)
>> -#define HDMITX_TOP_INTR_HPD_FALL BIT(2)
>> -#define HDMITX_TOP_INTR_RXSENSE_RISE BIT(6)
>> -#define HDMITX_TOP_INTR_RXSENSE_FALL BIT(7)
>> +#define HDMITX_TOP_INTR_STAT_CLR (0x014)
>> +#define TOP_INTR_CORE BIT(0)
>> +#define TOP_INTR_HPD_RISE BIT(1)
>> +#define TOP_INTR_HPD_FALL BIT(2)
>> +#define TOP_INTR_RXSENSE_RISE BIT(6)
>> +#define TOP_INTR_RXSENSE_FALL BIT(7)
>> /*
>> * Bit 14:12 RW tmds_sel: 3'b000=Output zero; 3'b001=Output normal TMDS data;
>> @@ -112,29 +115,31 @@
>> * 2=Output 1-bit pattern; 3=output 10-bit pattern. Default 0.
>> * Bit 0 RW prbs_pttn_en: 1=Enable PRBS generator; 0=Disable. Default 0.
>> */
>> -#define HDMITX_TOP_BIST_CNTL (0x006)
>> +#define HDMITX_TOP_BIST_CNTL (0x018)
>> +#define TOP_BIST_OUT_MASK GENMASK(14, 12)
>> +#define TOP_BIST_TMDS_EN BIT(12)
>> /* Bit 29:20 RW shift_pttn_data[59:50]. Default 0. */
>> /* Bit 19:10 RW shift_pttn_data[69:60]. Default 0. */
>> /* Bit 9: 0 RW shift_pttn_data[79:70]. Default 0. */
>> -#define HDMITX_TOP_SHIFT_PTTN_012 (0x007)
>> +#define HDMITX_TOP_SHIFT_PTTN_012 (0x01c)
>> /* Bit 29:20 RW shift_pttn_data[29:20]. Default 0. */
>> /* Bit 19:10 RW shift_pttn_data[39:30]. Default 0. */
>> /* Bit 9: 0 RW shift_pttn_data[49:40]. Default 0. */
>> -#define HDMITX_TOP_SHIFT_PTTN_345 (0x008)
>> +#define HDMITX_TOP_SHIFT_PTTN_345 (0x020)
>> /* Bit 19:10 RW shift_pttn_data[ 9: 0]. Default 0. */
>> /* Bit 9: 0 RW shift_pttn_data[19:10]. Default 0. */
>> -#define HDMITX_TOP_SHIFT_PTTN_67 (0x009)
>> +#define HDMITX_TOP_SHIFT_PTTN_67 (0x024)
>> /* Bit 25:16 RW tmds_clk_pttn[19:10]. Default 0. */
>> /* Bit 9: 0 RW tmds_clk_pttn[ 9: 0]. Default 0. */
>> -#define HDMITX_TOP_TMDS_CLK_PTTN_01 (0x00A)
>> +#define HDMITX_TOP_TMDS_CLK_PTTN_01 (0x028)
>> /* Bit 25:16 RW tmds_clk_pttn[39:30]. Default 0. */
>> /* Bit 9: 0 RW tmds_clk_pttn[29:20]. Default 0. */
>> -#define HDMITX_TOP_TMDS_CLK_PTTN_23 (0x00B)
>> +#define HDMITX_TOP_TMDS_CLK_PTTN_23 (0x02c)
>> /*
>> * Bit 1 RW shift_tmds_clk_pttn:1=Enable shifting clk pattern,
>> @@ -143,18 +148,22 @@
>> * [ 1] shift_tmds_clk_pttn
>> * [ 0] load_tmds_clk_pttn
>> */
>> -#define HDMITX_TOP_TMDS_CLK_PTTN_CNTL (0x00C)
>> +#define HDMITX_TOP_TMDS_CLK_PTTN_CNTL (0x030)
>> +#define TOP_TDMS_CLK_PTTN_LOAD BIT(0)
>> +#define TOP_TDMS_CLK_PTTN_SHFT BIT(1)
>> /*
>> * Bit 0 RW revocmem_wr_fail: Read back 1 to indicate Host write REVOC MEM
>> * failure, write 1 to clear the failure flag. Default 0.
>> */
>> -#define HDMITX_TOP_REVOCMEM_STAT (0x00D)
>> +#define HDMITX_TOP_REVOCMEM_STAT (0x034)
>> /*
>> * Bit 1 R filtered RxSense status
>> * Bit 0 R filtered HPD status.
>> */
>> -#define HDMITX_TOP_STAT0 (0x00E)
>> +#define HDMITX_TOP_STAT0 (0x038)
>> +#define TOP_STAT0_HPD BIT(0)
>> +#define TOP_STAT0_RXSENSE BIT(1)
>> #endif /* __MESON_DW_HDMI_H */
--
Jerome
