[PATCH 1/2] drm/meson: dw-hdmi: power up phy on device init
The phy is not in a useful state right after init. It will become useful,
including for auxiliary function such as CEC or ARC, after the first mode
is set. This is a problem on systems where the display is using another
interface like DSI or CVBS.
This change refactor the init and mode change callback to power up the PHY
on init and leave only what is necessary for mode changes in the related
function. This is enough to fix CEC operation when HDMI display is not
enabled.
Fixes: 3f68be7d8e96 ("drm/meson: Add support for HDMI encoder and DW-HDMI
bridge + PHY")
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/meson/meson_dw_hdmi.c | 51 +--
1 file changed, 17 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 5a9538bc0e26..a83d93078537 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -384,26 +384,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void
*data,
dw_hdmi_bus_fmt_is_420(hdmi))
mode_is_420 = true;
- /* Enable clocks */
- regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0x, 0x100);
-
- /* Bring HDMITX MEM output of power down */
- regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0);
-
- /* Bring out of reset */
- dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_SW_RESET, 0);
-
- /* Enable internal pixclk, tmds_clk, spdif_clk, i2s_clk, cecclk */
- dw_hdmi_top_write_bits(dw_hdmi, HDMITX_TOP_CLK_CNTL,
- 0x3, 0x3);
-
- /* Enable cec_clk and hdcp22_tmdsclk_en */
- dw_hdmi_top_write_bits(dw_hdmi, HDMITX_TOP_CLK_CNTL,
- 0x3 << 4, 0x3 << 4);
-
- /* Enable normal output to PHY */
- dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
-
/* TMDS pattern setup */
if (mode->clock > 34 && !mode_is_420) {
dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_TMDS_CLK_PTTN_01,
@@ -425,20 +405,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void
*data,
/* Setup PHY parameters */
meson_hdmi_phy_setup_mode(dw_hdmi, mode, mode_is_420);
- /* Setup PHY */
- regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
- 0x << 16, 0x0390 << 16);
-
- /* BIT_INVERT */
- if (dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
- dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
- dw_hdmi_is_compatible(dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
- regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
- BIT(17), 0);
- else
- regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
- BIT(17), BIT(17));
-
/* Disable clock, fifo, fifo_wr */
regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1, 0xf, 0);
@@ -656,6 +622,23 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi
*meson_dw_hdmi)
meson_dw_hdmi->data->top_write(meson_dw_hdmi,
HDMITX_TOP_CLK_CNTL, 0xff);
+ /* Enable normal output to PHY */
+ meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_BIST_CNTL,
BIT(12));
+
+ /* Setup PHY */
+ regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
+ 0x << 16, 0x0390 << 16);
+
+ /* BIT_INVERT */
+ if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
+ dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
+ dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
+ regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
+ BIT(17), 0);
+ else
+ regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
+ BIT(17), BIT(17));
+
/* Enable HDMI-TX Interrupt */
meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_INTR_STAT_CLR,
HDMITX_TOP_INTR_CORE);
--
2.43.0
[PATCH 2/2] drm/meson: dw-hdmi: add bandgap setting for g12
When no mode is set, the utility pin appears to be grounded. No signal
is getting through.
This is problematic because ARC and eARC use this line and may do so even
if no display mode is set.
This change enable the bandgap setting on g12 chip, which fix the problem
with the utility pin. This is done by restoring init values on PHY init and
disable.
Fixes: 3b7c1237a72a ("drm/meson: Add G12A support for the DW-HDMI Glue")
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ---
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index a83d93078537..5565f529 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -106,6 +106,8 @@
#define HHI_HDMI_CLK_CNTL 0x1cc /* 0x73 */
#define HHI_HDMI_PHY_CNTL0 0x3a0 /* 0xe8 */
#define HHI_HDMI_PHY_CNTL1 0x3a4 /* 0xe9 */
+#define PHY_CNTL1_INIT0x0390
+#define PHY_INVERTBIT(17)
#define HHI_HDMI_PHY_CNTL2 0x3a8 /* 0xea */
#define HHI_HDMI_PHY_CNTL3 0x3ac /* 0xeb */
#define HHI_HDMI_PHY_CNTL4 0x3b0 /* 0xec */
@@ -130,6 +132,8 @@ struct meson_dw_hdmi_data {
unsigned int addr);
void(*dwc_write)(struct meson_dw_hdmi *dw_hdmi,
unsigned int addr, unsigned int data);
+ u32 cntl0_init;
+ u32 cntl1_init;
};
struct meson_dw_hdmi {
@@ -458,7 +462,9 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi,
DRM_DEBUG_DRIVER("\n");
- regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, 0);
+ /* Fallback to init mode */
+ regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL1, dw_hdmi->data->cntl1_init);
+ regmap_write(priv->hhi, HHI_HDMI_PHY_CNTL0, dw_hdmi->data->cntl0_init);
}
static enum drm_connector_status dw_hdmi_read_hpd(struct dw_hdmi *hdmi,
@@ -576,11 +582,22 @@ static const struct regmap_config
meson_dw_hdmi_regmap_config = {
.fast_io = true,
};
-static const struct meson_dw_hdmi_data meson_dw_hdmi_gx_data = {
+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 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 meson_dw_hdmi_data meson_dw_hdmi_g12a_data = {
@@ -588,6 +605,8 @@ static const struct meson_dw_hdmi_data
meson_dw_hdmi_g12a_data = {
.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 void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi)
@@ -626,18 +645,8 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi
*meson_dw_hdmi)
meson_dw_hdmi->data->top_write(meson_dw_hdmi, HDMITX_TOP_BIST_CNTL,
BIT(12));
/* Setup PHY */
- regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
- 0x << 16, 0x0390 << 16);
-
- /* BIT_INVERT */
- if (dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxl-dw-hdmi") ||
- dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-gxm-dw-hdmi") ||
- dw_hdmi_is_compatible(meson_dw_hdmi, "amlogic,meson-g12a-dw-hdmi"))
- regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
- BIT(17), 0);
- else
- regmap_update_bits(priv->hhi, HHI_HDMI_PHY_CNTL1,
- BIT(17), BIT(17));
+ 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,
@@ -848,11 +857,11 @@ static const struct dev_pm_ops meson_dw_hdmi_pm_ops = {
static const struct of_device_id meson_dw_hdmi_of_table[] = {
{ .compatible = "amlogic,meson-gxbb-dw-hdmi",
- .data = _dw_hdmi_gx_data },
+ .data = _dw_hdmi_gxbb_data },
{ .compatible = "amlogic,meson-gxl-dw-hdmi",
- .data = _dw_hdmi_gx_data },
+ .data = _dw_hdmi_gxl_data },
{ .compatible = "amlogic,meson-gxm-dw-hdmi",
- .data = _dw_hdmi_gx_data },
+ .data = _dw_hdmi_gxl_data },
{ .compatible = "amlogic,meson-g12a-dw-hdmi",
.data = _dw_hdmi_g12a_data },
{ }
--
2.43.0
[PATCH 0/2] drm/meson: fix hdmi auxiliary system operation without display
CEC and ARC should work even when HDMI is not actively used for the display but it is not the case with Amlogic HDMI. This is important for devices such as sound bars which may use DSI to display a UI and HDMI for CEC/ARC. A display is not required for these functions This patchset fixes the problem. Jerome Brunet (2): drm/meson: dw-hdmi: power up phy on device init drm/meson: dw-hdmi: add bandgap setting for g12 drivers/gpu/drm/meson/meson_dw_hdmi.c | 70 --- 1 file changed, 31 insertions(+), 39 deletions(-) -- 2.43.0
Re: (subset) [PATCH v12 0/7] drm/meson: add support for MIPI DSI Display
Applied to clk-meson (v6.10/drivers), thanks! [2/7] clk: meson: add vclk driver https://github.com/BayLibre/clk-meson/commit/bb5aa08572b5 [3/7] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF https://github.com/BayLibre/clk-meson/commit/b70cb1a21a54 Best regards, -- Jerome
Re: [PATCH v12 2/7] clk: meson: add vclk driver
On Thu 04 Apr 2024 at 18:59, Neil Armstrong wrote:
> On 04/04/2024 10:13, Jerome Brunet wrote:
>> On Wed 03 Apr 2024 at 09:46, Neil Armstrong
>> wrote:
>>
>>> The VCLK and VCLK_DIV clocks have supplementary bits.
>>>
>>> The VCLK gate has a "SOFT RESET" bit to toggle after the whole
>>> VCLK sub-tree rate has been set, this is implemented in
>>> the gate enable callback.
>>>
>>> The VCLK_DIV clocks as enable and reset bits used to disable
>>> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
>>> the rate is set while the divider is disabled and in reset mode.
>>>
>>> The VCLK_DIV enable bit isn't implemented as a gate since it's part
>>> of the divider logic and vendor does this exact sequence to ensure
>>> the divider is correctly set.
>> The checkpatch warning is still there. Is it a choice or a mistake ?
>> Documentation says "GPL v2" exists for historic reason which seems to
>> hint "GPL" is preferred, and I suppose this is why checkpatch warns for
>> it.
>
> Well I didn't see this warning, this is what I fixed:
>
> $ scripts/checkpatch.pl --strict drivers/clk/meson/vclk.c
> CHECK: Alignment should match open parenthesis
> #63: FILE: drivers/clk/meson/vclk.c:63:
> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
> +unsigned long prate)
>
> CHECK: Alignment should match open parenthesis
> #73: FILE: drivers/clk/meson/vclk.c:73:
> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
>
> CHECK: Alignment should match open parenthesis
> #83: FILE: drivers/clk/meson/vclk.c:83:
> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
>
I would not ask a respin solely for this. It's nice to fix it but I was
mostly after the warning TBH.
>
>
> It seems that checking a commit triggers an extra check
>
> $ scripts/checkpatch.pl --strict -G 1bac9f6aa3c3
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #58:
> new file mode 100644
>
>
>
> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure
> the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
> #203: FILE: drivers/clk/meson/vclk.c:141:
> +MODULE_LICENSE("GPL v2");
Hum, I'm running checkpatch against the mail itself, not the commit. I
still get the warning
>
>
>
> Neil
>
>>
>>>
>>> Signed-off-by: Neil Armstrong
>>> ---
>>> drivers/clk/meson/Kconfig | 4 ++
>>> drivers/clk/meson/Makefile | 1 +
>>> drivers/clk/meson/vclk.c | 141
>>> +
>>> drivers/clk/meson/vclk.h | 51
>>> 4 files changed, 197 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index 29ffd14d267b..8a9823789fa3 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>> tristate
>>> select COMMON_CLK_MESON_REGMAP
>>> +config COMMON_CLK_MESON_VCLK
>>> + tristate
>>> + select COMMON_CLK_MESON_REGMAP
>>> +
>>> config COMMON_CLK_MESON_CLKC_UTILS
>>> tristate
>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>> index 9ee4b954c896..9ba43fe7a07a 100644
>>> --- a/drivers/clk/meson/Makefile
>>> +++ b/drivers/clk/meson/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>>> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>>> # Amlogic Clock controllers
>>> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
>>> new file mode 100644
>>> index ..45dc216941ea
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/vclk.c
>>> @@ -0,0 +1,141 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2024 Neil Armstrong
>>> + */
>>> +
>>> +#include
>>> +#include "vclk.h"
>>> +
>>>
Re: [PATCH v12 2/7] clk: meson: add vclk driver
On Wed 03 Apr 2024 at 09:46, Neil Armstrong wrote:
> The VCLK and VCLK_DIV clocks have supplementary bits.
>
> The VCLK gate has a "SOFT RESET" bit to toggle after the whole
> VCLK sub-tree rate has been set, this is implemented in
> the gate enable callback.
>
> The VCLK_DIV clocks as enable and reset bits used to disable
> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
> the rate is set while the divider is disabled and in reset mode.
>
> The VCLK_DIV enable bit isn't implemented as a gate since it's part
> of the divider logic and vendor does this exact sequence to ensure
> the divider is correctly set.
The checkpatch warning is still there. Is it a choice or a mistake ?
Documentation says "GPL v2" exists for historic reason which seems to
hint "GPL" is preferred, and I suppose this is why checkpatch warns for
it.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/Kconfig | 4 ++
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/vclk.c | 141
> +
> drivers/clk/meson/vclk.h | 51
> 4 files changed, 197 insertions(+)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 29ffd14d267b..8a9823789fa3 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
> tristate
> select COMMON_CLK_MESON_REGMAP
>
> +config COMMON_CLK_MESON_VCLK
> + tristate
> + select COMMON_CLK_MESON_REGMAP
> +
> config COMMON_CLK_MESON_CLKC_UTILS
> tristate
>
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 9ee4b954c896..9ba43fe7a07a 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>
> # Amlogic Clock controllers
>
> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
> new file mode 100644
> index ..45dc216941ea
> --- /dev/null
> +++ b/drivers/clk/meson/vclk.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Neil Armstrong
> + */
> +
> +#include
> +#include "vclk.h"
> +
> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
> +
> +static inline struct meson_vclk_gate_data *
> +clk_get_meson_vclk_gate_data(struct clk_regmap *clk)
> +{
> + return (struct meson_vclk_gate_data *)clk->data;
> +}
> +
> +static int meson_vclk_gate_enable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
> +
> + meson_parm_write(clk->map, >enable, 1);
> +
> + /* Do a reset pulse */
> + meson_parm_write(clk->map, >reset, 1);
> + meson_parm_write(clk->map, >reset, 0);
> +
> + return 0;
> +}
> +
> +static void meson_vclk_gate_disable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
> +
> + meson_parm_write(clk->map, >enable, 0);
> +}
> +
> +static int meson_vclk_gate_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
> +
> + return meson_parm_read(clk->map, >enable);
> +}
> +
> +const struct clk_ops meson_vclk_gate_ops = {
> + .enable = meson_vclk_gate_enable,
> + .disable = meson_vclk_gate_disable,
> + .is_enabled = meson_vclk_gate_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(meson_vclk_gate_ops);
> +
> +/* The VCLK Divider has supplementary reset & enable bits */
> +
> +static inline struct meson_vclk_div_data *
> +clk_get_meson_vclk_div_data(struct clk_regmap *clk)
> +{
> + return (struct meson_vclk_div_data *)clk->data;
> +}
> +
> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
> +
> + return divider_recalc_rate(hw, prate, meson_parm_read(clk->map,
> >div),
> +vclk->table, vclk->flags, vclk->div.width);
> +}
> +
> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
> +
> + return divider_determine_rate(hw, req, vclk->table, vclk->div.width,
> + vclk->flags);
> +}
> +
> +static int meson_vclk_div_set_rate(struct
Re: [PATCH v11 3/7] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF
On Mon 25 Mar 2024 at 12:09, Neil Armstrong wrote:
> In order to setup the DSI clock, let's make the unused VCLK2 clock path
> configuration via CCF.
>
> The nocache option is removed from following clocks:
> - vclk2_sel
> - vclk2_input
> - vclk2_div
> - vclk2
> - vclk_div1
> - vclk2_div2_en
> - vclk2_div4_en
> - vclk2_div6_en
> - vclk2_div12_en
> - vclk2_div2
> - vclk2_div4
> - vclk2_div6
> - vclk2_div12
> - cts_encl_sel
>
> vclk2 and vclk2_div uses the newly introduced vclk regmap driver
> to handle the enable and reset bits.
>
> In order to set a rate on cts_encl via the vclk2 clock path,
> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order
> to keep CCF from selection a parent.
> The parents of cts_encl_sel & vclk2_sel are expected to be defined
> in DT or manually set by the display driver at some point.
>
> The following clock scheme is to be used for DSI:
>
> xtal
> \_ gp0_pll_dco
>\_ gp0_pll
> |- vclk2_sel
> | \_ vclk2_input
> | \_ vclk2_div
> |\_ vclk2
> | \_ vclk2_div1
> | \_ cts_encl_sel
> | \_ cts_encl -> to VPU LCD Encoder
> |- mipi_dsi_pxclk_sel
> \_ mipi_dsi_pxclk_div
> \_ mipi_dsi_pxclk-> to DSI controller
>
> The mipi_dsi_pxclk_div is set as bypass with a single /1 entry in div_table
> in order to use the same GP0 for mipi_dsi_pxclk and vclk2_input.
>
> The SET_RATE_PARENT is only set on the mipi_dsi_pxclk_sel clock so the
> DSI bitclock is the reference base clock to calculate the vclk2_div value
> when pixel clock is set on the cts_encl endpoint.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/Kconfig | 1 +
> drivers/clk/meson/g12a.c | 72
> ++-
> 2 files changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 8a9823789fa3..59a40a49f8e1 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -144,6 +144,7 @@ config COMMON_CLK_G12A
> select COMMON_CLK_MESON_EE_CLKC
> select COMMON_CLK_MESON_CPU_DYNDIV
> select COMMON_CLK_MESON_VID_PLL_DIV
> + select COMMON_CLK_MESON_VCLK
> select MFD_SYSCON
> help
> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 90f4c6103014..083882e53b65 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -22,6 +22,7 @@
> #include "clk-regmap.h"
> #include "clk-cpu-dyndiv.h"
> #include "vid-pll-div.h"
> +#include "vclk.h"
> #include "meson-eeclk.h"
> #include "g12a.h"
>
> @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = {
> .ops = _regmap_mux_ops,
> .parent_hws = g12a_vclk_parent_hws,
> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws),
> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_NO_REPARENT,
> },
> };
>
> @@ -3193,7 +3194,6 @@ static struct clk_regmap g12a_vclk2_input = {
> .ops = _regmap_gate_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2_sel.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> },
> };
>
> @@ -3215,19 +3215,32 @@ static struct clk_regmap g12a_vclk_div = {
> };
>
> static struct clk_regmap g12a_vclk2_div = {
> - .data = &(struct clk_regmap_div_data){
> - .offset = HHI_VIID_CLK_DIV,
> - .shift = 0,
> - .width = 8,
> + .data = &(struct meson_vclk_div_data){
> + .div = {
> + .reg_off = HHI_VIID_CLK_DIV,
> + .shift = 0,
> + .width = 8,
> + },
> + .enable = {
> + .reg_off = HHI_VIID_CLK_DIV,
> + .shift = 16,
> + .width = 1,
> + },
> + .reset = {
> + .reg_off = HHI_VIID_CLK_DIV,
> + .shift = 17,
> + .width = 1,
> + },
> + .flags = CLK_DIVIDER_ROUND_CLOSEST,
> },
> .hw.init = &(struct clk_init_data){
> .name = "vclk2_div",
> - .ops = _regmap_divider_ops,
> + .ops = _vclk_div_ops,
> .parent_hws = (const struct clk_hw *[]) {
> _vclk2_input.hw
> },
> .num_parents = 1,
> - .flags = CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_GATE,
> },
> };
>
> @@ -3246,16 +3259,24 @@ static struct clk_regmap g12a_vclk = {
> };
>
> static struct clk_regmap g12a_vclk2 = {
> - .data = &(struct clk_regmap_gate_data){
> - .offset = HHI_VIID_CLK_CNTL,
> - .bit_idx = 19,
> + .data =
Re: [PATCH v11 2/7] clk: meson: add vclk driver
On Mon 25 Mar 2024 at 12:09, Neil Armstrong wrote:
> The VCLK and VCLK_DIV clocks have supplementary bits.
>
> The VCLK gate has a "SOFT RESET" bit to toggle after the whole
> VCLK sub-tree rate has been set, this is implemented in
> the gate enable callback.
>
> The VCLK_DIV clocks as enable and reset bits used to disable
> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
> the rate is set while the divider is disabled and in reset mode.
>
> The VCLK_DIV enable bit isn't implemented as a gate since it's part
> of the divider logic and vendor does this exact sequence to ensure
> the divider is correctly set.
checkpatch reports a few easy CHECKs and one WARNING.
Could you please fix these ?
Other than that, It looks OK.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/Kconfig | 4 ++
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/vclk.c | 141
> +
> drivers/clk/meson/vclk.h | 51
> 4 files changed, 197 insertions(+)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 29ffd14d267b..8a9823789fa3 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
> tristate
> select COMMON_CLK_MESON_REGMAP
>
> +config COMMON_CLK_MESON_VCLK
> + tristate
> + select COMMON_CLK_MESON_REGMAP
> +
> config COMMON_CLK_MESON_CLKC_UTILS
> tristate
>
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 9ee4b954c896..9ba43fe7a07a 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>
> # Amlogic Clock controllers
>
> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
> new file mode 100644
> index ..3ea813a0a995
> --- /dev/null
> +++ b/drivers/clk/meson/vclk.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024 Neil Armstrong
> + */
> +
> +#include
> +#include "vclk.h"
> +
> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
> +
> +static inline struct meson_vclk_gate_data *
> +clk_get_meson_vclk_gate_data(struct clk_regmap *clk)
> +{
> + return (struct meson_vclk_gate_data *)clk->data;
> +}
> +
> +static int meson_vclk_gate_enable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
> +
> + meson_parm_write(clk->map, >enable, 1);
> +
> + /* Do a reset pulse */
> + meson_parm_write(clk->map, >reset, 1);
> + meson_parm_write(clk->map, >reset, 0);
> +
> + return 0;
> +}
> +
> +static void meson_vclk_gate_disable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
> +
> + meson_parm_write(clk->map, >enable, 0);
> +}
> +
> +static int meson_vclk_gate_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
> +
> + return meson_parm_read(clk->map, >enable);
> +}
> +
> +const struct clk_ops meson_vclk_gate_ops = {
> + .enable = meson_vclk_gate_enable,
> + .disable = meson_vclk_gate_disable,
> + .is_enabled = meson_vclk_gate_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(meson_vclk_gate_ops);
> +
> +/* The VCLK Divider has supplementary reset & enable bits */
> +
> +static inline struct meson_vclk_div_data *
> +clk_get_meson_vclk_div_data(struct clk_regmap *clk)
> +{
> + return (struct meson_vclk_div_data *)clk->data;
> +}
> +
> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
> +
> + return divider_recalc_rate(hw, prate, meson_parm_read(clk->map,
> >div),
> +vclk->table, vclk->flags, vclk->div.width);
> +}
> +
> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
> +
> + return divider_determine_rate(hw, req, vclk->table, vclk->div.width,
> + vclk->flags);
> +}
> +
> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
On Wed 13 Dec 2023 at 17:44, Neil Armstrong wrote: > Hi Maxime, > > Le 13/12/2023 à 09:36, Maxime Ripard a écrit : >> Hi, >> On Wed, Dec 13, 2023 at 08:43:00AM +0100, Uwe Kleine-König wrote: >>> On Wed, Dec 13, 2023 at 08:16:04AM +0100, Maxime Ripard wrote: On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote: > clk_rate_exclusive_get() returns zero unconditionally. Most users "know" > that and don't check the return value. This series fixes the four users > that do error checking on the returned value and then makes function > return void. > > Given that the changes to the drivers are simple and so merge conflicts > (if any) should be easy to handle, I suggest to merge this complete > series via the clk tree. I don't think it's the right way to go about it. clk_rate_exclusive_get() should be expected to fail. For example if there's another user getting an exclusive rate on the same clock. If we're not checking for it right now, then it should probably be fixed, but the callers checking for the error are right to do so if they rely on an exclusive rate. It's the ones that don't that should be modified. >>> >>> If some other consumer has already "locked" a clock that I call >>> clk_rate_exclusive_get() for, this isn't an error. In my bubble I call >>> this function because I don't want the rate to change e.g. because I >>> setup some registers in the consuming device to provide a fixed UART >>> baud rate or i2c bus frequency (and that works as expected). >> I guess it's a larger conversation, but I don't see how that can >> possibly work. >> The way the API is designed, you have no guarantee (outside of >> clk_rate_exclusive_*) that the rate is going to change. >> And clk_rate_exclusive_get() doesn't allow the rate to change while in >> the "critical section". >> So the only possible thing to do is clk_set_rate() + >> clk_rate_exclusive_get(). > > There's clk_set_rate_exclusive() for this purpose. > >> So there's a window where the clock can indeed be changed, and the >> consumer that is about to lock its rate wouldn't be aware of it. >> I guess it would work if you don't care about the rate at all, you just >> want to make sure it doesn't change. >> Out of the 7 users of that function, 3 are in that situation, so I guess >> it's fair. >> 3 are open to that race condition I mentioned above. >> 1 is calling clk_set_rate while in the critical section, which works if >> there's a single user but not if there's multiple, so it should be >> discouraged. >> >>> In this case I won't be able to change the rate of the clock, but that >>> is signalled by clk_set_rate() failing (iff and when I need awother >>> rate) which also seems the right place to fail to me. >> Which is ignored by like half the callers, including the one odd case I >> mentioned above. >> And that's super confusing still: you can *always* get exclusivity, but >> not always do whatever you want with the rate when you have it? How are >> drivers supposed to recover from that? You can handle failing to get >> exclusivity, but certainly not working around variable guarantees. >> >>> It's like that since clk_rate_exclusive_get() was introduced in 2017 >>> (commit 55e9b8b7b806ec3f9a8817e13596682a5981c19c). >> Right, but "it's always been that way" surely can't be an argument, >> otherwise you wouldn't have done that series in the first place. >> >>> BTW, I just noticed that my assertion "Most users \"know\" that >>> [clk_rate_exclusive_get() returns zero unconditionally]" is wrong. As of >>> next-20231213 there are 3 callers ignoring the return value of >>> clk_rate_exclusive_get() and 4 that handle (imaginary) returned errors. >>> I expected this function to be used more extensively. (In fact I think >>> it should be used more as several drivers rely on the clk rate not >>> changing.) >> Yes, but also it's super difficult to use in practice, and most devices >> don't care. >> The current situation is something like this: >>* Only a handful of devices really care about their clock rate, and >> often only for one of their clock if they have several. You would >> probably get all the devices that create an analog signal somehow >> there, so audio, display, i2c, spi, uarts, etc. Plus the ones doing >> frequency scaling so CPU and GPUs. >>* CPUs and GPUs are very likely to have a dedicated clock, so we can >> rule the "another user is going to mess with my clock" case. >>* UARTs/i2c/etc. are usually taking their clock from the bus interface >> directly which is pretty much never going to change (for good >> reason). And the rate of the bus is not really likely to change. >>* SPI/NAND/MMC usually have their dedicated clock too, and the bus >> rate is not likely to change after the initial setup either. >> So, the only affected devices are the ones generating external signals, >> with the rate
Re: [PATCH 0/5] clk: Make clk_rate_exclusive_get() return void
On Wed 13 Dec 2023 at 08:16, Maxime Ripard wrote: > [[PGP Signed Part:Undecided]] > Hi, > > On Tue, Dec 12, 2023 at 06:26:37PM +0100, Uwe Kleine-König wrote: >> Hello, >> >> clk_rate_exclusive_get() returns zero unconditionally. Most users "know" >> that and don't check the return value. This series fixes the four users >> that do error checking on the returned value and then makes function >> return void. >> >> Given that the changes to the drivers are simple and so merge conflicts >> (if any) should be easy to handle, I suggest to merge this complete >> series via the clk tree. > > I don't think it's the right way to go about it. > > clk_rate_exclusive_get() should be expected to fail. Yes, at some point it might. That is why the API returns an error code. What CCF (or any other framework) should be no concern to the consummer. Driver not checking the return are taking there chances, and that is up to them. It is like regmap. Most calls can return an error code but the vast majority of driver happily ignore that. > For example if > there's another user getting an exclusive rate on the same clock. > > If we're not checking for it right now, then it should probably be > fixed, but the callers checking for the error are right to do so if they > rely on an exclusive rate. It's the ones that don't that should be > modified. > I'm not sure that would be right. For sure, restricting a to single user was not my intent when I wrote the thing. The intent was for a consumer to state that it cannot tolerate a rate change of the clock it is using. It is fine for several consumers to state that for a single clock, as long as they 'agree' on the rate. Two instances of the same device could be a good example of that. Those consumers should use 'clk_set_rate_exclusive()' to set the rate and protect it atomically. Calling 'clk_exclusive_get()' then 'clk_set_rate()' is racy as both instance could effectively lock the rate without actually getting the rate they want :/ Admittingly, the API naming is terrible when it comes to this ... > Maxime > > [[End of PGP Signed Part]] -- Jerome
Re: [PATCH v9 07/12] clk: meson: add vclk driver
On Mon 27 Nov 2023 at 17:14, Neil Armstrong wrote:
> On 24/11/2023 15:41, Jerome Brunet wrote:
>> On Fri 24 Nov 2023 at 09:41, Neil Armstrong
>> wrote:
>>
>>> The VCLK and VCLK_DIV clocks have supplementary bits.
>>>
>>> The VCLK has a "SOFT RESET" bit to toggle after the whole
>>> VCLK sub-tree rate has been set, this is implemented in
>>> the gate enable callback.
>>>
>>> The VCLK_DIV clocks as enable and reset bits used to disable
>>> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
>>> the rate is set while the divider is disabled and in reset mode.
>>>
>>> The VCLK_DIV enable bit isn't implemented as a gate since it's part
>>> of the divider logic and vendor does this exact sequence to ensure
>>> the divider is correctly set.
>>>
>>> Signed-off-by: Neil Armstrong
>>> ---
>>> drivers/clk/meson/Kconfig | 5 ++
>>> drivers/clk/meson/Makefile | 1 +
>>> drivers/clk/meson/vclk.c | 141
>>> +
>>> drivers/clk/meson/vclk.h | 51
>>> 4 files changed, 198 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index 29ffd14d267b..59a40a49f8e1 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>> tristate
>>> select COMMON_CLK_MESON_REGMAP
>>> +config COMMON_CLK_MESON_VCLK
>>> + tristate
>>> + select COMMON_CLK_MESON_REGMAP
>>> +
>>> config COMMON_CLK_MESON_CLKC_UTILS
>>> tristate
>>> @@ -140,6 +144,7 @@ config COMMON_CLK_G12A
>>> select COMMON_CLK_MESON_EE_CLKC
>>> select COMMON_CLK_MESON_CPU_DYNDIV
>>> select COMMON_CLK_MESON_VID_PLL_DIV
>>> + select COMMON_CLK_MESON_VCLK
>> This particular line belong in the next patch
>>
>>> select MFD_SYSCON
>>> help
>>> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>> index 9ee4b954c896..9ba43fe7a07a 100644
>>> --- a/drivers/clk/meson/Makefile
>>> +++ b/drivers/clk/meson/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>>> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>>> # Amlogic Clock controllers
>>> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
>>> new file mode 100644
>>> index ..47f08a52b49f
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/vclk.c
>>> @@ -0,0 +1,141 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2023 Neil Armstrong
>>> + */
>>> +
>>> +#include
>>> +#include "vclk.h"
>>> +
>>> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
>>> +
>>> +static inline struct clk_regmap_vclk_data *
>>> +clk_get_regmap_vclk_data(struct clk_regmap *clk)
>>> +{
>>> + return (struct clk_regmap_vclk_data *)clk->data;
>>> +}
>>> +
>>> +static int clk_regmap_vclk_enable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
>>> +
>>> + meson_parm_write(clk->map, >enable, 1);
>>> +
>>> + /* Do a reset pulse */
>>> + meson_parm_write(clk->map, >reset, 1);
>>> + meson_parm_write(clk->map, >reset, 0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void clk_regmap_vclk_disable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
>>> +
>>> + meson_parm_write(clk->map, >enable, 0);
>>> +}
>>> +
>>> +static int clk_regmap_vclk_is_enabled(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
>>&
Re: [PATCH v9 08/12] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF
>> >>> >>> I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk >>> ratios, >>> since it doesn't exist on AXG. Not sure we would ever need it... and none >>> of the other upstream DSI drivers supports such setups. >>> >>> The main reasons I set only mipi_dsi_pxclk in DT is because : >>> 1) the DSI controller requires a bitclk to respond, pclk is not enough >>> 2) GP0 is disabled with an invalid config at cold boot, thus we cannot >>> rely on a default/safe rate on an initial prepare_enable(). >>> This permits setting initial valid state for the DSI controller, while >>> the actual bitclk and vclk are calculated dynamically with panel/bridge >>> runtime parameters. >> Nothing against setting rate in DT when it is static. Setting it then >> overriding it is not easy to follow. > > Yup, would be simpler to only have parenting set in DT, since it must > stay static, I'm fine trying to move rate setup to code. > >> To work around GP0 not being set, assuming you want to keep rate >> propagation as it is, you could call clk_set_rate() on cts_encl (possibly w/o >> enabling it) to force a setup on gp0 then clk_prepare_enable() on >> pxclk. You'd get a your safe rate on GP0 and the clock you need on pxclk. >> It is a bit hackish. Might be better to claim gp0 in your driver to >> manage it directly, cutting rate propagation above it to control each >> branch of the subtree as you need. It seems you need to have control over >> that anyway and it would be clear GP0 is expected to belong to DSI. > > Controlling the PLL from the DSI controller seems violating too much layers, > DSI controller driver is not feed directly by the PLL so it's a non-sense > regarding DT properties. Not sure what you mean by this. You have shown in your the commit message that the DSI clocks make significant subtree. I don't see a problem if you need to manage the root of that subtree. I'd be great if you didn't need to, but it is what it is ... > > Setting a safe clock from the DSI controller probe is an idea, but again I > don't know which value I should use... You mentionned that the problem comes DSI bridges that needs to change at runtime. I don't know much about those TBH, but is there anyway you can come up with a static GP0 rate that would then be able to divide to serve all the rates bridge would need in your use case ? GP0 can go a lot higher than ~100MHz and there are dividers unused in the tree it seems. I suppose there is a finite number of required rate for each use case ? If there are not too many and there is a common divider that allows a common rate GP0 can do, it would solve your problem. It's a lot of if but It is worth checking. This is how audio works and DT assigned rate is a good match in this case. > > I'll review the clk parenting flags and try to hack something. > > Thanks, > Neil > >
Re: (subset) [PATCH v9 00/12] drm/meson: add support for MIPI DSI Display
Applied to clk-meson (v6.8/drivers), thanks! [01/12] dt-bindings: clk: g12a-clkc: add CTS_ENCL clock ids https://github.com/BayLibre/clk-meson/commit/bd5ef3f21d17 [06/12] clk: meson: g12a: add CTS_ENCL & CTS_ENCL_SEL clocks https://github.com/BayLibre/clk-meson/commit/5de4e8353e32 Best regards, -- Jerome
Re: [PATCH v9 08/12] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF
On Fri 24 Nov 2023 at 16:15, Neil Armstrong wrote: > On 24/11/2023 15:12, Jerome Brunet wrote: >> On Fri 24 Nov 2023 at 09:41, Neil Armstrong >> wrote: >> >>> In order to setup the DSI clock, let's make the unused VCLK2 clock path >>> configuration via CCF. >>> >>> The nocache option is removed from following clocks: >>> - vclk2_sel >>> - vclk2_input >>> - vclk2_div >>> - vclk2 >>> - vclk_div1 >>> - vclk2_div2_en >>> - vclk2_div4_en >>> - vclk2_div6_en >>> - vclk2_div12_en >>> - vclk2_div2 >>> - vclk2_div4 >>> - vclk2_div6 >>> - vclk2_div12 >>> - cts_encl_sel >>> >>> vclk2 and vclk2_div uses the newly introduced vclk regmap driver >>> to handle the enable and reset bits. >>> >>> In order to set a rate on cts_encl via the vclk2 clock path, >>> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order >>> to keep CCF from selection a parent. >>> The parents of cts_encl_sel & vclk2_sel are expected to be defined >>> in DT. >>> >>> The following clock scheme is to be used for DSI: >>> >>> xtal >>> \_ gp0_pll_dco >>> \_ gp0_pll >>>|- vclk2_sel >>>| \_ vclk2_input >>>| \_ vclk2_div >>>|\_ vclk2 >>>| \_ vclk2_div1 >>>| \_ cts_encl_sel >>>| \_ cts_encl-> to VPU LCD Encoder >>>|- mipi_dsi_pxclk_sel >>>\_ mipi_dsi_pxclk_div >>> \_ mipi_dsi_pxclk -> to DSI controller >>> >>> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0 >>> for mipi_dsi_pxclk and vclk2_input. >> Could you explain a bit more this part of about the RO ops ? >> Maybe I'm missing something. >> You would be relying on the reset being always the way it. It is >> probable but not safe. >> A way to deal with the shared GP0 would be to: >> * cut rate propagation at mipi_dsi_pxclk_sel (already done) and >>(vclk2_sel - TBD) ... >> * Set GP0 base rate through assigned-clock-rate (which you already in >>patch 11) >> With this, I'm not sure anything needs to be RO for the rates to be set >> properly for each subtree. >> Also, with the subtree above and your example in patch 11, it looks odd >> that >> PXCLK is manually set through DT while ENCL is not. Both are input of >> dsi driver. > > So the deal is about dynamic setup of clocks for DSI bridges, not really > for panels where we can probably know in advance the clock setup. > > In this particular case, we need to keep a ratio between the vclk and the > DSI bitclk, the DSI bitclk is taken from mipi_dsi_pxclk and vclk is derived > from gp0 via vclk2. > > If we set the bitclk rate via mipi_dsi_pxclk, CCF will try to use > mipi_dsi_pxclk_div > to achieve the rate, If you have CLK_RATE_PARENT on mipi_dsi_pxclk_sel, I'm not surprised it does that, but you don't :/ I'm quite surprised it would do that, or even could. >From your example setting 96Mhz on both gp0 and mipi_dsi_pxclk, since you've proposed RO-OPS, I suppose the divider is assumed to be 1 and stay like that forever. With rate propagation disabled mipi_dsi_pxclk_sel and GP0 is 96Mhz, CCF would have no choice but picking 1 as divider, so I don't understand how CCF would pick anything else and how RO-OPS help > and it does it everytime I tried, breaking the vclk/bitclk ratio, > and we have no way to know the gp0 rate in this case. If you really want to ensure the divider value is always 1, why not use a divider table allowing only 1 ? Adding a comment in the g12 clock driver would nice because this not obvious. It would be safer than relying on the reset value. > > I suspect mipi_dsi_pxclk_div was added to achieve fractional vclk/bitclk > ratios, > since it doesn't exist on AXG. Not sure we would ever need it... and none > of the other upstream DSI drivers supports such setups. > > The main reasons I set only mipi_dsi_pxclk in DT is because : > 1) the DSI controller requires a bitclk to respond, pclk is not enough > 2) GP0 is disabled with an invalid config at cold boot, thus we cannot > rely on a default/safe rate on an initial prepare_enable(). > This permits setting initial valid state for the DSI controller, while > the actual bitclk and vclk are calculated dynamically with panel/bridge > runtime parameters. Nothing against setting rate in DT when it is static. Setting it then overriding it is not easy to follow. To work around GP0 not being set
Re: [PATCH v9 07/12] clk: meson: add vclk driver
On Fri 24 Nov 2023 at 09:41, Neil Armstrong wrote:
> The VCLK and VCLK_DIV clocks have supplementary bits.
>
> The VCLK has a "SOFT RESET" bit to toggle after the whole
> VCLK sub-tree rate has been set, this is implemented in
> the gate enable callback.
>
> The VCLK_DIV clocks as enable and reset bits used to disable
> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
> the rate is set while the divider is disabled and in reset mode.
>
> The VCLK_DIV enable bit isn't implemented as a gate since it's part
> of the divider logic and vendor does this exact sequence to ensure
> the divider is correctly set.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/Kconfig | 5 ++
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/vclk.c | 141
> +
> drivers/clk/meson/vclk.h | 51
> 4 files changed, 198 insertions(+)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 29ffd14d267b..59a40a49f8e1 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
> tristate
> select COMMON_CLK_MESON_REGMAP
>
> +config COMMON_CLK_MESON_VCLK
> + tristate
> + select COMMON_CLK_MESON_REGMAP
> +
> config COMMON_CLK_MESON_CLKC_UTILS
> tristate
>
> @@ -140,6 +144,7 @@ config COMMON_CLK_G12A
> select COMMON_CLK_MESON_EE_CLKC
> select COMMON_CLK_MESON_CPU_DYNDIV
> select COMMON_CLK_MESON_VID_PLL_DIV
> + select COMMON_CLK_MESON_VCLK
This particular line belong in the next patch
> select MFD_SYSCON
> help
> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 9ee4b954c896..9ba43fe7a07a 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>
> # Amlogic Clock controllers
>
> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
> new file mode 100644
> index ..47f08a52b49f
> --- /dev/null
> +++ b/drivers/clk/meson/vclk.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 Neil Armstrong
> + */
> +
> +#include
> +#include "vclk.h"
> +
> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
> +
> +static inline struct clk_regmap_vclk_data *
> +clk_get_regmap_vclk_data(struct clk_regmap *clk)
> +{
> + return (struct clk_regmap_vclk_data *)clk->data;
> +}
> +
> +static int clk_regmap_vclk_enable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
> +
> + meson_parm_write(clk->map, >enable, 1);
> +
> + /* Do a reset pulse */
> + meson_parm_write(clk->map, >reset, 1);
> + meson_parm_write(clk->map, >reset, 0);
> +
> + return 0;
> +}
> +
> +static void clk_regmap_vclk_disable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
> +
> + meson_parm_write(clk->map, >enable, 0);
> +}
> +
> +static int clk_regmap_vclk_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
> +
> + return meson_parm_read(clk->map, >enable);
> +}
> +
> +const struct clk_ops clk_regmap_vclk_ops = {
> + .enable = clk_regmap_vclk_enable,
> + .disable = clk_regmap_vclk_disable,
> + .is_enabled = clk_regmap_vclk_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_vclk_ops);
s/clk_regmap_vclk/meson_vclk at least for what is exported, ideally most
all the code.
I get clk_regmap_ comes from code copied from clk_regmap.c.
The reason the this part is different (and not using parm) if that when
I converted amlogic to regmap, I hope we could make this generic,
possibly converging between aml and qcom (which was the only other
platform using regmap for clock at the time). This is why clk_regmap.c
is a bit different from the other driver.
For the aml specific drivers, best to look at the mpll or cpu-dyndiv one.
> +
> +/* The VCLK Divider has supplementary reset & enable bits */
> +
> +static inline struct clk_regmap_vclk_div_data *
> +clk_get_regmap_vclk_div_data(struct clk_regmap *clk)
> +{
> + return (struct clk_regmap_vclk_div_data *)clk->data;
> +}
> +
> +static unsigned long clk_regmap_vclk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> +
Re: [PATCH v9 08/12] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF
On Fri 24 Nov 2023 at 09:41, Neil Armstrong wrote:
> In order to setup the DSI clock, let's make the unused VCLK2 clock path
> configuration via CCF.
>
> The nocache option is removed from following clocks:
> - vclk2_sel
> - vclk2_input
> - vclk2_div
> - vclk2
> - vclk_div1
> - vclk2_div2_en
> - vclk2_div4_en
> - vclk2_div6_en
> - vclk2_div12_en
> - vclk2_div2
> - vclk2_div4
> - vclk2_div6
> - vclk2_div12
> - cts_encl_sel
>
> vclk2 and vclk2_div uses the newly introduced vclk regmap driver
> to handle the enable and reset bits.
>
> In order to set a rate on cts_encl via the vclk2 clock path,
> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order
> to keep CCF from selection a parent.
> The parents of cts_encl_sel & vclk2_sel are expected to be defined
> in DT.
>
> The following clock scheme is to be used for DSI:
>
> xtal
> \_ gp0_pll_dco
>\_ gp0_pll
> |- vclk2_sel
> | \_ vclk2_input
> | \_ vclk2_div
> |\_ vclk2
> | \_ vclk2_div1
> | \_ cts_encl_sel
> | \_ cts_encl -> to VPU LCD Encoder
> |- mipi_dsi_pxclk_sel
> \_ mipi_dsi_pxclk_div
> \_ mipi_dsi_pxclk-> to DSI controller
>
> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0
> for mipi_dsi_pxclk and vclk2_input.
Could you explain a bit more this part of about the RO ops ?
Maybe I'm missing something.
You would be relying on the reset being always the way it. It is
probable but not safe.
A way to deal with the shared GP0 would be to:
* cut rate propagation at mipi_dsi_pxclk_sel (already done) and
(vclk2_sel - TBD) ...
* Set GP0 base rate through assigned-clock-rate (which you already in
patch 11)
With this, I'm not sure anything needs to be RO for the rates to be set
properly for each subtree.
Also, with the subtree above and your example in patch 11, it looks odd that
PXCLK is manually set through DT while ENCL is not. Both are input of
dsi driver.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/g12a.c | 68
> +---
> 1 file changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index cadd824336ad..fb3d9196a1fd 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -22,6 +22,7 @@
> #include "clk-regmap.h"
> #include "clk-cpu-dyndiv.h"
> #include "vid-pll-div.h"
> +#include "vclk.h"
> #include "meson-eeclk.h"
> #include "g12a.h"
>
> @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = {
> .ops = _regmap_mux_ops,
> .parent_hws = g12a_vclk_parent_hws,
> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws),
> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
No sure CLK_SET_RATE_PARENT is wise here.
What you manually set in DT for the GP0, is likely to change because of
this, isn't it ?
> },
> };
>
> @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = {
> .ops = _regmap_gate_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2_sel.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + .flags = CLK_SET_RATE_PARENT,
> },
> };
>
> @@ -3215,19 +3216,32 @@ static struct clk_regmap g12a_vclk_div = {
> };
>
> static struct clk_regmap g12a_vclk2_div = {
> - .data = &(struct clk_regmap_div_data){
> - .offset = HHI_VIID_CLK_DIV,
> - .shift = 0,
> - .width = 8,
> + .data = &(struct clk_regmap_vclk_div_data){
> + .div = {
> + .reg_off = HHI_VIID_CLK_DIV,
> + .shift = 0,
> + .width = 8,
> + },
> + .enable = {
> + .reg_off = HHI_VIID_CLK_DIV,
> + .shift = 16,
> + .width = 1,
> + },
> + .reset = {
> + .reg_off = HHI_VIID_CLK_DIV,
> + .shift = 17,
> + .width = 1,
> + },
> + .flags = CLK_DIVIDER_ROUND_CLOSEST,
> },
> .hw.init = &(struct clk_init_data){
> .name = "vclk2_div",
> - .ops = _regmap_divider_ops,
> + .ops = _regmap_vclk_div_ops,
> .parent_hws = (const struct clk_hw *[]) {
> _vclk2_input.hw
> },
> .num_parents = 1,
> - .flags = CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
> },
> };
>
> @@ -3246,16 +3260,24 @@ static struct clk_regmap g12a_vclk = {
> };
>
> static struct clk_regmap g12a_vclk2 = {
> - .data = &(struct clk_regmap_gate_data){
> - .offset =
Re: [PATCH v7 4/9] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF
On Thu 03 Aug 2023 at 14:03, Neil Armstrong wrote:
> In order to setup the DSI clock, let's make the unused VCLK2 clock path
> configuration via CCF.
>
> The nocache option is removed from following clocks:
> - vclk2_sel
> - vclk2_input
> - vclk2_div
> - vclk2
> - vclk_div1
> - vclk2_div2_en
> - vclk2_div4_en
> - vclk2_div6_en
> - vclk2_div12_en
> - vclk2_div2
> - vclk2_div4
> - vclk2_div6
> - vclk2_div12
> - cts_encl_sel
>
> vclk2 and vclk2_div uses the newly introduced vclk regmap driver
> to handle the enable and reset bits.
>
> In order to set a rate on cts_encl via the vclk2 clock path,
> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order
> to keep CCF from selection a parent.
> The parents of cts_encl_sel & vclk2_sel are expected to be defined
> in DT.
>
> The following clock scheme is to be used for DSI:
>
> xtal
> \_ gp0_pll_dco
>\_ gp0_pll
> |- vclk2_sel
> | \_ vclk2_input
> | \_ vclk2_div
> |\_ vclk2
> | \_ vclk2_div1
> | \_ cts_encl_sel
> | \_ cts_encl -> to VPU LCD Encoder
> |- mipi_dsi_pxclk_sel
> \_ mipi_dsi_pxclk_div
> \_ mipi_dsi_pxclk-> to DSI controller
>
> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0
> for mipi_dsi_pxclk and vclk2_input.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/g12a.c | 43 ++-
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 5d62134335c1..552c8efb1ad8 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -22,6 +22,7 @@
> #include "clk-regmap.h"
> #include "clk-cpu-dyndiv.h"
> #include "vid-pll-div.h"
> +#include "vclk.h"
> #include "meson-eeclk.h"
> #include "g12a.h"
>
> @@ -3165,7 +3166,7 @@ static struct clk_regmap g12a_vclk2_sel = {
> .ops = _regmap_mux_ops,
> .parent_hws = g12a_vclk_parent_hws,
> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws),
> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
> },
> };
>
> @@ -3193,7 +3194,7 @@ static struct clk_regmap g12a_vclk2_input = {
> .ops = _regmap_gate_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2_sel.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + .flags = CLK_SET_RATE_PARENT,
... oh, I see. Ignore the comment patch #2
> },
> };
>
> @@ -3215,19 +3216,22 @@ static struct clk_regmap g12a_vclk_div = {
> };
>
> static struct clk_regmap g12a_vclk2_div = {
> - .data = &(struct clk_regmap_div_data){
> + .data = &(struct clk_regmap_vclk_div_data){
> .offset = HHI_VIID_CLK_DIV,
> .shift = 0,
> .width = 8,
> + .enable_bit_idx = 16,
> + .reset_bit_idx = 17,
> + .flags = CLK_DIVIDER_ROUND_CLOSEST,
> },
> .hw.init = &(struct clk_init_data){
> .name = "vclk2_div",
> - .ops = _regmap_divider_ops,
> + .ops = _regmap_vclk_div_ops,
> .parent_hws = (const struct clk_hw *[]) {
> _vclk2_input.hw
> },
> .num_parents = 1,
> - .flags = CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
> },
> };
>
> @@ -3246,16 +3250,17 @@ static struct clk_regmap g12a_vclk = {
> };
>
> static struct clk_regmap g12a_vclk2 = {
> - .data = &(struct clk_regmap_gate_data){
> + .data = &(struct clk_regmap_vclk_data){
> .offset = HHI_VIID_CLK_CNTL,
> - .bit_idx = 19,
> + .enable_bit_idx = 19,
> + .reset_bit_idx = 15,
> },
> .hw.init = &(struct clk_init_data) {
> .name = "vclk2",
> - .ops = _regmap_gate_ops,
> + .ops = _regmap_vclk_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2_div.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
> },
> };
>
> @@ -3339,7 +3344,7 @@ static struct clk_regmap g12a_vclk2_div1 = {
> .ops = _regmap_gate_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + .flags = CLK_SET_RATE_PARENT,
> },
> };
>
> @@ -3353,7 +3358,7 @@ static struct clk_regmap g12a_vclk2_div2_en = {
> .ops = _regmap_gate_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT |
Re: [PATCH v7 2/9] clk: meson: g12a: add CTS_ENCL & CTS_ENCL_SEL clocks
On Thu 03 Aug 2023 at 14:03, Neil Armstrong wrote:
> Add new CTS_ENCL & CTS_ENCL_SEL clocks for the G12A compatible
> SoCs, they are used to feed the VPU LCD Pixel encoder used for
> DSI display purposes.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/g12a.c | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index ceabd5f4b2ac..5d62134335c1 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -3549,6 +3549,22 @@ static struct clk_regmap g12a_cts_encp_sel = {
> },
> };
>
> +static struct clk_regmap g12a_cts_encl_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VIID_CLK_DIV,
> + .mask = 0xf,
> + .shift = 12,
> + .table = mux_table_cts_sel,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "cts_encl_sel",
> + .ops = _regmap_mux_ops,
> + .parent_hws = g12a_cts_parent_hws,
> + .num_parents = ARRAY_SIZE(g12a_cts_parent_hws),
> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE,
Why nocache ?
This is usually used when the consumer driver is poking around behind
CCF back.
Any chance this can use assigned-parent or CCF directly ?
> + },
> +};
> +
> static struct clk_regmap g12a_cts_vdac_sel = {
> .data = &(struct clk_regmap_mux_data){
> .offset = HHI_VIID_CLK_DIV,
> @@ -3628,6 +3644,22 @@ static struct clk_regmap g12a_cts_encp = {
> },
> };
>
> +static struct clk_regmap g12a_cts_encl = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VID_CLK_CNTL2,
> + .bit_idx = 3,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "cts_encl",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) {
> + _cts_encl_sel.hw
> + },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
What is the reason for IGNORE_UNUSED ?
If you need to keep the clock on while the driver comes up, please
document it here.
> + },
> +};
> +
> static struct clk_regmap g12a_cts_vdac = {
> .data = &(struct clk_regmap_gate_data){
> .offset = HHI_VID_CLK_CNTL2,
> @@ -4407,10 +4439,12 @@ static struct clk_hw *g12a_hw_clks[] = {
> [CLKID_VCLK2_DIV12] = _vclk2_div12.hw,
> [CLKID_CTS_ENCI_SEL]= _cts_enci_sel.hw,
> [CLKID_CTS_ENCP_SEL]= _cts_encp_sel.hw,
> + [CLKID_CTS_ENCL_SEL]= _cts_encl_sel.hw,
> [CLKID_CTS_VDAC_SEL]= _cts_vdac_sel.hw,
> [CLKID_HDMI_TX_SEL] = _hdmi_tx_sel.hw,
> [CLKID_CTS_ENCI]= _cts_enci.hw,
> [CLKID_CTS_ENCP]= _cts_encp.hw,
> + [CLKID_CTS_ENCL]= _cts_encl.hw,
> [CLKID_CTS_VDAC]= _cts_vdac.hw,
> [CLKID_HDMI_TX] = _hdmi_tx.hw,
> [CLKID_HDMI_SEL]= _hdmi_sel.hw,
> @@ -4632,10 +4666,12 @@ static struct clk_hw *g12b_hw_clks[] = {
> [CLKID_VCLK2_DIV12] = _vclk2_div12.hw,
> [CLKID_CTS_ENCI_SEL]= _cts_enci_sel.hw,
> [CLKID_CTS_ENCP_SEL]= _cts_encp_sel.hw,
> + [CLKID_CTS_ENCL_SEL]= _cts_encl_sel.hw,
> [CLKID_CTS_VDAC_SEL]= _cts_vdac_sel.hw,
> [CLKID_HDMI_TX_SEL] = _hdmi_tx_sel.hw,
> [CLKID_CTS_ENCI]= _cts_enci.hw,
> [CLKID_CTS_ENCP]= _cts_encp.hw,
> + [CLKID_CTS_ENCL]= _cts_encl.hw,
> [CLKID_CTS_VDAC]= _cts_vdac.hw,
> [CLKID_HDMI_TX] = _hdmi_tx.hw,
> [CLKID_HDMI_SEL]= _hdmi_sel.hw,
> @@ -4892,10 +4928,12 @@ static struct clk_hw *sm1_hw_clks[] = {
> [CLKID_VCLK2_DIV12] = _vclk2_div12.hw,
> [CLKID_CTS_ENCI_SEL]= _cts_enci_sel.hw,
> [CLKID_CTS_ENCP_SEL]= _cts_encp_sel.hw,
> + [CLKID_CTS_ENCL_SEL]= _cts_encl_sel.hw,
> [CLKID_CTS_VDAC_SEL]= _cts_vdac_sel.hw,
> [CLKID_HDMI_TX_SEL] = _hdmi_tx_sel.hw,
> [CLKID_CTS_ENCI]= _cts_enci.hw,
> [CLKID_CTS_ENCP]= _cts_encp.hw,
> + [CLKID_CTS_ENCL]= _cts_encl.hw,
> [CLKID_CTS_VDAC]= _cts_vdac.hw,
> [CLKID_HDMI_TX] = _hdmi_tx.hw,
> [CLKID_HDMI_SEL]= _hdmi_sel.hw,
> @@ -5123,10 +5161,12 @@ static struct clk_regmap *const g12a_clk_regmaps[] = {
> _vclk2_div12_en,
> _cts_enci_sel,
> _cts_encp_sel,
> + _cts_encl_sel,
> _cts_vdac_sel,
> _hdmi_tx_sel,
> _cts_enci,
> _cts_encp,
> + _cts_encl,
> _cts_vdac,
>
Re: [PATCH v7 3/9] clk: meson: add vclk driver
On Thu 03 Aug 2023 at 14:03, Neil Armstrong wrote:
> The VCLK and VCLK_DIV clocks have supplementary bits.
>
> The VCLK has a "SOFT RESET" bit to toggle after the whole
> VCLK sub-tree rate has been set, this is implemented in
> the gate enable callback.
>
> The VCLK_DIV clocks as enable and reset bits used to disable
> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
> the rate is set while the divider is disabled and in reset mode.
>
> The VCLK_DIV enable bit isn't implemented as a gate since it's part
> of the divider logic and vendor does this exact sequence to ensure
> the divider is correctly set.
Unless there is reason, I'd prefer if this driver was using 'struct
parm', like the rest of amlogic custom clock drivers, for consistency.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/Kconfig | 5 ++
> drivers/clk/meson/Makefile | 1 +
> drivers/clk/meson/vclk.c | 146
> +
> drivers/clk/meson/vclk.h | 68 +
> 4 files changed, 220 insertions(+)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 135da8f2d0b1..83f629515e96 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
> tristate
> select COMMON_CLK_MESON_REGMAP
>
> +config COMMON_CLK_MESON_VCLK
> + tristate
> + select COMMON_CLK_MESON_REGMAP
> +
> config COMMON_CLK_MESON_CLKC_UTILS
> tristate
>
> @@ -140,6 +144,7 @@ config COMMON_CLK_G12A
> select COMMON_CLK_MESON_EE_CLKC
> select COMMON_CLK_MESON_CPU_DYNDIV
> select COMMON_CLK_MESON_VID_PLL_DIV
> + select COMMON_CLK_MESON_VCLK
> select MFD_SYSCON
> help
> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index cd961cc4f4db..6efeb8c7bd2a 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>
> # Amlogic Clock controllers
>
> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
> new file mode 100644
> index ..0df84403b17f
> --- /dev/null
> +++ b/drivers/clk/meson/vclk.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 Neil Armstrong
> + */
> +
> +#include
> +#include "vclk.h"
> +
> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
> +
> +static int clk_regmap_vclk_enable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
> +
> + regmap_set_bits(clk->map, vclk->offset, BIT(vclk->enable_bit_idx));
> +
> + /* Do a reset pulse */
> + regmap_set_bits(clk->map, vclk->offset, BIT(vclk->reset_bit_idx));
> + regmap_clear_bits(clk->map, vclk->offset, BIT(vclk->reset_bit_idx));
> +
> + return 0;
> +}
> +
> +static void clk_regmap_vclk_disable(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
> +
> + regmap_clear_bits(clk->map, vclk->offset, BIT(vclk->enable_bit_idx));
> +}
> +
> +static int clk_regmap_vclk_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk);
> + unsigned int val;
> +
> + regmap_read(clk->map, vclk->offset, );
> +
> + return val & BIT(vclk->enable_bit_idx) ? 1 : 0;
> +}
> +
> +const struct clk_ops clk_regmap_vclk_ops = {
> + .enable = clk_regmap_vclk_enable,
> + .disable = clk_regmap_vclk_disable,
> + .is_enabled = clk_regmap_vclk_is_enabled,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_vclk_ops);
> +
> +/* The VCLK Divider has supplementary reset & enable bits */
> +
> +static unsigned long clk_regmap_vclk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
> +{
> + struct clk_regmap *clk = to_clk_regmap(hw);
> + struct clk_regmap_vclk_div_data *vclk =
> clk_get_regmap_vclk_div_data(clk);
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(clk->map, vclk->offset, );
> + if (ret)
> + /* Gives a hint that something is wrong */
> + return 0;
> +
> + val >>= vclk->shift;
> + val &= clk_div_mask(vclk->width);
> +
> + return divider_recalc_rate(hw, prate, val, vclk->table, vclk->flags,
> +vclk->width);
> +}
> +
> +static int clk_regmap_vclk_div_determine_rate(struct clk_hw *hw,
> +
Re: [PATCH v5 01/17] clk: meson: g12a: prefix private CLK IDs defines with PRIV
On Tue 30 May 2023 at 17:56, Neil Armstrong wrote:
> On 30/05/2023 10:08, Jerome Brunet wrote:
>> On Tue 30 May 2023 at 09:38, Neil Armstrong
>> wrote:
>>
>>> Exposing should not be done in a single commit anymore due to
>>> dt-bindings enforced rules.
>>>
>>> Prepend PRIV to the private CLK IDs so we can add new clock to
>>> the bindings header and in a separate commit remove such private
>>> define and switch to the public CLK IDs identifier.
>>>
>>> This refers to a discussion at [1] with Arnd and Krzysztof.
>>>
>>> [1]
>>> https://lore.kernel.org/all/2fabe721-7434-43e7-bae5-088a42ba1...@app.fastmail.com/
>>>
>>> Signed-off-by: Neil Armstrong
>> I understand the discussion reported but I don't really like this
>> CLKID_PRIV_
>> It adds another layer of IDs.
>> I'd much prefer if we just expose all the IDs. That would comply with DT
>> new policy and be much simpler in the long run.
>
> While it would solve everything at long term, we'll still need to do the move
> in 3 steps (add PRIV, add to bindings, remove PRIV defined), and we should
> still
It would certainly be a lot simpler if we could expose the IDs like we used
to one last time to comply with this new requirement.
If it is really not possible, then yes, we will have no choice but to
bounce using this namespace trick. If there is no other choice, then I'd
prefer if it was done for all the IDs of the different SoCs, once and for all.
> decide how to handle NR_CLKS.
>
Can't this stay in the driver header ? This needs to be updated only the
actually adding the clock, isn't it ?
Maybe I'm missing something ...
> Neil
>
>>
>>> ---
>>> drivers/clk/meson/g12a.c | 628
>>> +++
>>> drivers/clk/meson/g12a.h | 260 ++--
>>> 2 files changed, 444 insertions(+), 444 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
>>> index 310accf94830..d2e481ae2429 100644
>>> --- a/drivers/clk/meson/g12a.c
>>> +++ b/drivers/clk/meson/g12a.c
>>> @@ -4255,8 +4255,8 @@ static struct clk_hw_onecell_data
>>> g12a_hw_onecell_data = {
>>> [CLKID_FCLK_DIV7] = _fclk_div7.hw,
>>> [CLKID_FCLK_DIV2P5] = _fclk_div2p5.hw,
>>> [CLKID_GP0_PLL] = _gp0_pll.hw,
>>> - [CLKID_MPEG_SEL]= _mpeg_clk_sel.hw,
>>> - [CLKID_MPEG_DIV]= _mpeg_clk_div.hw,
>>> + [CLKID_PRIV_MPEG_SEL] = _mpeg_clk_sel.hw,
>>> + [CLKID_PRIV_MPEG_DIV] = _mpeg_clk_div.hw,
>>> [CLKID_CLK81] = _clk81.hw,
>>> [CLKID_MPLL0] = _mpll0.hw,
>>> [CLKID_MPLL1] = _mpll1.hw,
>>> @@ -4307,25 +4307,25 @@ static struct clk_hw_onecell_data
>>> g12a_hw_onecell_data = {
>>> [CLKID_UART2] = _uart2.hw,
>>> [CLKID_VPU_INTR]= _vpu_intr.hw,
>>> [CLKID_GIC] = _gic.hw,
>>> - [CLKID_SD_EMMC_A_CLK0_SEL] = _sd_emmc_a_clk0_sel.hw,
>>> - [CLKID_SD_EMMC_A_CLK0_DIV] = _sd_emmc_a_clk0_div.hw,
>>> + [CLKID_PRIV_SD_EMMC_A_CLK0_SEL] = _sd_emmc_a_clk0_sel.hw,
>>> + [CLKID_PRIV_SD_EMMC_A_CLK0_DIV] = _sd_emmc_a_clk0_div.hw,
>>> [CLKID_SD_EMMC_A_CLK0] = _sd_emmc_a_clk0.hw,
>>> - [CLKID_SD_EMMC_B_CLK0_SEL] = _sd_emmc_b_clk0_sel.hw,
>>> - [CLKID_SD_EMMC_B_CLK0_DIV] = _sd_emmc_b_clk0_div.hw,
>>> + [CLKID_PRIV_SD_EMMC_B_CLK0_SEL] = _sd_emmc_b_clk0_sel.hw,
>>> + [CLKID_PRIV_SD_EMMC_B_CLK0_DIV] = _sd_emmc_b_clk0_div.hw,
>>> [CLKID_SD_EMMC_B_CLK0] = _sd_emmc_b_clk0.hw,
>>> - [CLKID_SD_EMMC_C_CLK0_SEL] = _sd_emmc_c_clk0_sel.hw,
>>> - [CLKID_SD_EMMC_C_CLK0_DIV] = _sd_emmc_c_clk0_div.hw,
>>> + [CLKID_PRIV_SD_EMMC_C_CLK0_SEL] = _sd_emmc_c_clk0_sel.hw,
>>> + [CLKID_PRIV_SD_EMMC_C_CLK0_DIV] = _sd_emmc_c_clk0_div.hw,
>>> [CLKID_SD_EMMC_C_CLK0] = _sd_emmc_c_clk0.hw,
>>> - [CLKID_MPLL0_DIV] = _mpll0_div.hw,
>>> - [CLKID_MPLL1_DIV] = _mpll1_div.hw,
>>> - [CLKID_MPLL2_DIV] = _mpll2_div.hw,
>>> - [CLKID_MPLL3_DIV]
Re: [PATCH v5 05/17] clk: meson: g12a: make VCLK2 and ENCL clock path configurable by CCF
On Tue 30 May 2023 at 09:38, Neil Armstrong wrote:
> In order to setup the DSI clock, let's make the unused VCLK2 clock path
> configuration via CCF.
>
> The nocache option is removed from following clocks:
> - vclk2_sel
> - vclk2_input
> - vclk2_div
> - vclk2
> - vclk_div1
> - vclk2_div2_en
> - vclk2_div4_en
> - vclk2_div6_en
> - vclk2_div12_en
> - vclk2_div2
> - vclk2_div4
> - vclk2_div6
> - vclk2_div12
> - cts_encl_sel
>
> The missing vclk2 reset sequence is handled via new clkc notifiers
> in order to reset the vclk2 after each rate change as done by Amlogic
> in the vendor implementation.
>
> In order to set a rate on cts_encl via the vclk2 clock path,
> the NO_REPARENT flag is set on cts_encl_sel & vclk2_sel in order
> to keep CCF from selection a parent.
> The parents of cts_encl_sel & vclk2_sel are expected to be defined
> in DT.
>
> The following clock scheme is to be used for DSI:
>
> xtal
> \_ gp0_pll_dco
>\_ gp0_pll
> |- vclk2_sel
> | \_ vclk2_input
> | \_ vclk2_div
> |\_ vclk2
> | \_ vclk2_div1
> | \_ cts_encl_sel
> | \_ cts_encl -> to VPU LCD Encoder
> |- mipi_dsi_pxclk_sel
> \_ mipi_dsi_pxclk_div
> \_ mipi_dsi_pxclk-> to DSI controller
>
> The mipi_dsi_pxclk_div is set as RO in order to use the same GP0
> for mipi_dsi_pxclk and vclk2_input.
I don't think notifiers is the appropriate approach here.
Whenever there is clock change the motifiers would trigger an off/on of
the clock, regardless of the clock usage or state.
If you have several consummers on this vclk2, this would
cause glitches and maybe this is not desirable.
I think it would be better to handle the enable and reset with a
specific gate driver, in prepare() or enable(), and the give the clock
CLK_SET_RATE_GATE flag.
This would require the clock to be properly turn off before changing the
rate.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/g12a.c | 131
> +++
> 1 file changed, 120 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 461ebd79497c..e4053f4957d5 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -3163,7 +3163,7 @@ static struct clk_regmap g12a_vclk2_sel = {
> .ops = _regmap_mux_ops,
> .parent_hws = g12a_vclk_parent_hws,
> .num_parents = ARRAY_SIZE(g12a_vclk_parent_hws),
> - .flags = CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE,
> + .flags = CLK_SET_RATE_NO_REPARENT,
> },
> };
>
> @@ -3191,7 +3191,6 @@ static struct clk_regmap g12a_vclk2_input = {
> .ops = _regmap_gate_ops,
> .parent_hws = (const struct clk_hw *[]) { _vclk2_sel.hw },
> .num_parents = 1,
> - .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> },
> };
>
> @@ -3212,6 +3211,40 @@ static struct clk_regmap g12a_vclk_div = {
> },
> };
>
> +struct g12a_vclk_div_notifier {
> + struct clk_regmap *clk;
> + unsigned int offset;
> + u8 en_bit_idx;
> + u8 reset_bit_idx;
> + struct notifier_block nb;
> +};
> +
> +static int g12a_vclk_div_notifier_cb(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct g12a_vclk_div_notifier *nb_data =
> + container_of(nb, struct g12a_vclk_div_notifier, nb);
> +
> + switch (event) {
> + case PRE_RATE_CHANGE:
> + /* disable and reset vclk2 divider */
> + regmap_update_bits(nb_data->clk->map, nb_data->offset,
> +BIT(nb_data->en_bit_idx) |
> +BIT(nb_data->reset_bit_idx),
> +BIT(nb_data->reset_bit_idx));
> + return NOTIFY_OK;
> + case POST_RATE_CHANGE:
> + /* enabled and release reset */
> + regmap_update_bits(nb_data->clk->map, nb_data->offset,
> +BIT(nb_data->en_bit_idx) |
> +BIT(nb_data->reset_bit_idx),
> +BIT(nb_data->en_bit_idx));
> + return NOTIFY_OK;
> + default:
> + return NOTIFY_DONE;
> + };
> +};
> +
> static struct clk_regmap g12a_vclk2_div = {
> .data = &(struct clk_regmap_div_data){
> .offset = HHI_VIID_CLK_DIV,
> @@ -3225,10 +3258,18 @@ static struct clk_regmap g12a_vclk2_div = {
> _vclk2_input.hw
> },
> .num_parents = 1,
> - .flags = CLK_GET_RATE_NOCACHE,
> + .flags = CLK_DIVIDER_ROUND_CLOSEST,
> },
> };
>
> +static struct g12a_vclk_div_notifier g12a_vclk2_div_data = {
> + .clk = _vclk2_div,
> + .offset = HHI_VIID_CLK_DIV,
> + .en_bit_idx = 16,
> + .reset_bit_idx = 17,
> + .nb.notifier_call =
Re: [PATCH v5 01/17] clk: meson: g12a: prefix private CLK IDs defines with PRIV
On Tue 30 May 2023 at 09:38, Neil Armstrong wrote:
> Exposing should not be done in a single commit anymore due to
> dt-bindings enforced rules.
>
> Prepend PRIV to the private CLK IDs so we can add new clock to
> the bindings header and in a separate commit remove such private
> define and switch to the public CLK IDs identifier.
>
> This refers to a discussion at [1] with Arnd and Krzysztof.
>
> [1]
> https://lore.kernel.org/all/2fabe721-7434-43e7-bae5-088a42ba1...@app.fastmail.com/
>
> Signed-off-by: Neil Armstrong
I understand the discussion reported but I don't really like this CLKID_PRIV_
It adds another layer of IDs.
I'd much prefer if we just expose all the IDs. That would comply with DT
new policy and be much simpler in the long run.
> ---
> drivers/clk/meson/g12a.c | 628
> +++
> drivers/clk/meson/g12a.h | 260 ++--
> 2 files changed, 444 insertions(+), 444 deletions(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 310accf94830..d2e481ae2429 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -4255,8 +4255,8 @@ static struct clk_hw_onecell_data g12a_hw_onecell_data
> = {
> [CLKID_FCLK_DIV7] = _fclk_div7.hw,
> [CLKID_FCLK_DIV2P5] = _fclk_div2p5.hw,
> [CLKID_GP0_PLL] = _gp0_pll.hw,
> - [CLKID_MPEG_SEL]= _mpeg_clk_sel.hw,
> - [CLKID_MPEG_DIV]= _mpeg_clk_div.hw,
> + [CLKID_PRIV_MPEG_SEL] = _mpeg_clk_sel.hw,
> + [CLKID_PRIV_MPEG_DIV] = _mpeg_clk_div.hw,
> [CLKID_CLK81] = _clk81.hw,
> [CLKID_MPLL0] = _mpll0.hw,
> [CLKID_MPLL1] = _mpll1.hw,
> @@ -4307,25 +4307,25 @@ static struct clk_hw_onecell_data
> g12a_hw_onecell_data = {
> [CLKID_UART2] = _uart2.hw,
> [CLKID_VPU_INTR]= _vpu_intr.hw,
> [CLKID_GIC] = _gic.hw,
> - [CLKID_SD_EMMC_A_CLK0_SEL] = _sd_emmc_a_clk0_sel.hw,
> - [CLKID_SD_EMMC_A_CLK0_DIV] = _sd_emmc_a_clk0_div.hw,
> + [CLKID_PRIV_SD_EMMC_A_CLK0_SEL] = _sd_emmc_a_clk0_sel.hw,
> + [CLKID_PRIV_SD_EMMC_A_CLK0_DIV] = _sd_emmc_a_clk0_div.hw,
> [CLKID_SD_EMMC_A_CLK0] = _sd_emmc_a_clk0.hw,
> - [CLKID_SD_EMMC_B_CLK0_SEL] = _sd_emmc_b_clk0_sel.hw,
> - [CLKID_SD_EMMC_B_CLK0_DIV] = _sd_emmc_b_clk0_div.hw,
> + [CLKID_PRIV_SD_EMMC_B_CLK0_SEL] = _sd_emmc_b_clk0_sel.hw,
> + [CLKID_PRIV_SD_EMMC_B_CLK0_DIV] = _sd_emmc_b_clk0_div.hw,
> [CLKID_SD_EMMC_B_CLK0] = _sd_emmc_b_clk0.hw,
> - [CLKID_SD_EMMC_C_CLK0_SEL] = _sd_emmc_c_clk0_sel.hw,
> - [CLKID_SD_EMMC_C_CLK0_DIV] = _sd_emmc_c_clk0_div.hw,
> + [CLKID_PRIV_SD_EMMC_C_CLK0_SEL] = _sd_emmc_c_clk0_sel.hw,
> + [CLKID_PRIV_SD_EMMC_C_CLK0_DIV] = _sd_emmc_c_clk0_div.hw,
> [CLKID_SD_EMMC_C_CLK0] = _sd_emmc_c_clk0.hw,
> - [CLKID_MPLL0_DIV] = _mpll0_div.hw,
> - [CLKID_MPLL1_DIV] = _mpll1_div.hw,
> - [CLKID_MPLL2_DIV] = _mpll2_div.hw,
> - [CLKID_MPLL3_DIV] = _mpll3_div.hw,
> - [CLKID_FCLK_DIV2_DIV] = _fclk_div2_div.hw,
> - [CLKID_FCLK_DIV3_DIV] = _fclk_div3_div.hw,
> - [CLKID_FCLK_DIV4_DIV] = _fclk_div4_div.hw,
> - [CLKID_FCLK_DIV5_DIV] = _fclk_div5_div.hw,
> - [CLKID_FCLK_DIV7_DIV] = _fclk_div7_div.hw,
> - [CLKID_FCLK_DIV2P5_DIV] = _fclk_div2p5_div.hw,
> + [CLKID_PRIV_MPLL0_DIV] = _mpll0_div.hw,
> + [CLKID_PRIV_MPLL1_DIV] = _mpll1_div.hw,
> + [CLKID_PRIV_MPLL2_DIV] = _mpll2_div.hw,
> + [CLKID_PRIV_MPLL3_DIV] = _mpll3_div.hw,
> + [CLKID_PRIV_FCLK_DIV2_DIV] = _fclk_div2_div.hw,
> + [CLKID_PRIV_FCLK_DIV3_DIV] = _fclk_div3_div.hw,
> + [CLKID_PRIV_FCLK_DIV4_DIV] = _fclk_div4_div.hw,
> + [CLKID_PRIV_FCLK_DIV5_DIV] = _fclk_div5_div.hw,
> + [CLKID_PRIV_FCLK_DIV7_DIV] = _fclk_div7_div.hw,
> + [CLKID_PRIV_FCLK_DIV2P5_DIV]= _fclk_div2p5_div.hw,
> [CLKID_HIFI_PLL]= _hifi_pll.hw,
> [CLKID_VCLK2_VENCI0]= _vclk2_venci0.hw,
> [CLKID_VCLK2_VENCI1]= _vclk2_venci1.hw,
> @@ -4346,56 +4346,56 @@ static struct clk_hw_onecell_data
> g12a_hw_onecell_data = {
> [CLKID_VCLK2_VENCLMMC] = _vclk2_venclmmc.hw,
> [CLKID_VCLK2_VENCL]
Re: [PATCH v4 01/13] dt-bindings: clk: g12a-clkc: export VCLK2_SEL and add CTS_ENCL clock ids
On Tue 16 May 2023 at 11:00, Neil Armstrong wrote: > On 16/05/2023 10:44, Arnd Bergmann wrote: >> On Mon, May 15, 2023, at 18:22, neil.armstr...@linaro.org wrote: >>> On 15/05/2023 18:15, Krzysztof Kozlowski wrote: On 15/05/2023 18:13, Krzysztof Kozlowski wrote: Also one more argument maybe not relevant here but for other cases - this makes literally impossible to include the clock ID in DTS in the same kernel revision, because you must not merge driver branch to DTS branch. SoC folks were complaining about this many times. >>> >>> Actually we handle this very simply by having such patches merged in a >>> immutable >>> branch merged in the clock and DT pull-requests, it worked perfectly so far >>> and neither Stephen or Arnd complained about that. >> It's usually benign if you just add a new clk at the end of the binding >> header, as that doesn't touch the internal header file in the same >> commit. I'm certainly happier about drivers that just use numbers from >> a datasheet instead of having to come up with numbers to stick in a binding >> because the hardware is entirely irregular, but there is usually no point >> trying to complain about bad hardware to the driver authors -- I unsterstand >> you are just trying to make things work. >> I agree with Krzysztof that using the same identifiers in the local >> header and in the binding is just making your life harder for no >> reason, and if you are the only ones doing it this way, it would >> help to change it. Maybe just add a namespace prefix to all the internal >> macros so the next time you move one into the documented bindings you >> can do it with the same immutable branch hack but not include the >> driver changes in the dt branch. > > Ack, I'll try to find a simple intermediate solution to avoid this situation. I'd in favor of keeping things simple and just put all the IDs in the bindings. We have been doodling with the idea for while, I think now is the time > > Thanks, > Neil > >> Arnd
Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
On Fri 20 Nov 2020 at 10:42, Marc Zyngier wrote:
> The HDMI driver request clocks early, but never disable them, leaving
> the clocks on even when the driver is removed.
>
> Fix it by slightly refactoring the clock code, and register a devm
> action that will eventually disable/unprepare the enabled clocks.
>
> Signed-off-by: Marc Zyngier
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ++-
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7f8eea494147..29623b309cb1 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -145,8 +145,6 @@ struct meson_dw_hdmi {
> struct reset_control *hdmitx_apb;
> struct reset_control *hdmitx_ctrl;
> struct reset_control *hdmitx_phy;
> - struct clk *hdmi_pclk;
> - struct clk *venci_clk;
> struct regulator *hdmi_supply;
> u32 irq_stat;
> struct dw_hdmi *hdmi;
> @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data)
> regulator_disable(data);
> }
>
> +static void meson_disable_clk(void *data)
> +{
> + clk_disable_unprepare(data);
> +}
> +
> +static int meson_enable_clk(struct device *dev, char *name)
> +{
> + struct clk *clk;
> + int ret;
> +
> + clk = devm_clk_get(dev, name);
> + if (IS_ERR(clk)) {
> + dev_err(dev, "Unable to get %s pclk\n", name);
> + return PTR_ERR(clk);
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (!ret)
> + ret = devm_add_action_or_reset(dev, meson_disable_clk,
> clk);
Thanks for fixing this Marc.
FYI, while it is fine to declare a function to disable the clocks, a quick
cast may avoid it
devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, clk);
> +
> + return ret;
> +}
> +
> static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> void *data)
> {
> @@ -1026,19 +1047,13 @@ static int meson_dw_hdmi_bind(struct device *dev,
> struct device *master,
> if (IS_ERR(meson_dw_hdmi->hdmitx))
> return PTR_ERR(meson_dw_hdmi->hdmitx);
>
> - meson_dw_hdmi->hdmi_pclk = devm_clk_get(dev, "isfr");
> - if (IS_ERR(meson_dw_hdmi->hdmi_pclk)) {
> - dev_err(dev, "Unable to get HDMI pclk\n");
> - return PTR_ERR(meson_dw_hdmi->hdmi_pclk);
> - }
> - clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);
> + ret = meson_enable_clk(dev, "isfr");
> + if (ret)
> + return ret;
>
> - meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci");
> - if (IS_ERR(meson_dw_hdmi->venci_clk)) {
> - dev_err(dev, "Unable to get venci clk\n");
> - return PTR_ERR(meson_dw_hdmi->venci_clk);
> - }
> - clk_prepare_enable(meson_dw_hdmi->venci_clk);
> + ret = meson_enable_clk(dev, "venci");
> + if (ret)
> + return ret;
>
> dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi,
> _dw_hdmi_regmap_config);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200
On Thu 19 Nov 2020 at 19:04, Guillaume Tucker
wrote:
> Hi Marc,
>
> On 19/11/2020 11:58, Marc Zyngier wrote:
>> On 2020-11-19 10:26, Neil Armstrong wrote:
>>> On 19/11/2020 11:20, Marc Zyngier wrote:
On 2020-11-19 08:50, Guillaume Tucker wrote:
> Please see the automated bisection report below about some kernel
> errors on meson-gxbb-p200.
>
> Reports aren't automatically sent to the public while we're
> trialing new bisection features on kernelci.org, however this one
> looks valid.
>
> The bisection started with next-20201118 but the errors are still
> present in next-20201119. Details for this regression:
>
> https://kernelci.org/test/case/id/5fb6196bfd0127fd68d8d902/
>
> The first error is:
>
> [ 14.757489] Internal error: synchronous external abort: 96000210
> [#1] PREEMPT SMP
Looks like yet another clock ordering setup. I guess different Amlogic
platforms have slightly different ordering requirements.
Neil, do you have any idea of which platform requires which ordering?
The variability in DT and platforms is pretty difficult to follow (and
I don't think I have such board around).
>>>
>>> The requirements should be the same, here the init was done before calling
>>> dw_hdmi_probe to be sure the clocks and internals resets were deasserted.
>>> But since you boot from u-boot already enabling these, it's already active.
>>>
>>> The solution would be to revert and do some check in meson_dw_hdmi_init() to
>>> check if already enabled and do nothing.
>>
>> A better fix seems to be this, which makes it explicit that there is
>> a dependency between some of the registers accessed from meson_dw_hdmi_init()
>> and the iahb clock.
>>
>> Guillaume, can you give this a go on your failing box?
>
> I confirm it solves the problem. Please add this to your fix
> patch if it's OK with you:
>
> Reported-by: "kernelci.org bot"
> Tested-by: Guillaume Tucker
>
>
> For the record, it passed all the tests when applied on top of
> the "bad" revision found by the bisection:
>
>
> http://lava.baylibre.com:10080/scheduler/alljobs?search=v5.10-rc3-1021-gb8668a2e5ea1
>
> and the exact same test on the "bad" revision without the fix
> consistently showed the error:
>
> http://lava.baylibre.com:10080/scheduler/job/374176
>
>
> Thanks,
> Guillaume
>
>
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index 7f8eea494147..52af8ba94311 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -146,6 +146,7 @@ struct meson_dw_hdmi {
>> struct reset_control *hdmitx_ctrl;
>> struct reset_control *hdmitx_phy;
>> struct clk *hdmi_pclk;
>> +struct clk *iahb_clk;
>> struct clk *venci_clk;
>> struct regulator *hdmi_supply;
>> u32 irq_stat;
>> @@ -1033,6 +1034,13 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>> }
>> clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);
>>
>> +meson_dw_hdmi->iahb_clk = devm_clk_get(dev, "iahb");
>> +if (IS_ERR(meson_dw_hdmi->iahb_clk)) {
>> +dev_err(dev, "Unable to get iahb clk\n");
>> +return PTR_ERR(meson_dw_hdmi->iahb_clk);
>> +}
>> +clk_prepare_enable(meson_dw_hdmi->iahb_clk);
If you guys are going ahead with this fix, this call to
clk_prepare_enable() needs to be balanced with clk_disable_unprepare() somehow
>> +
>> meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci");
>> if (IS_ERR(meson_dw_hdmi->venci_clk)) {
>> dev_err(dev, "Unable to get venci clk\n");
>> @@ -1071,6 +1079,8 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>>
>> encoder->possible_crtcs = BIT(0);
>>
>> +meson_dw_hdmi_init(meson_dw_hdmi);
>> +
>> DRM_DEBUG_DRIVER("encoder initialized\n");
>>
>> /* Bridge / Connector */
>> @@ -1095,8 +1105,6 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>> if (IS_ERR(meson_dw_hdmi->hdmi))
>> return PTR_ERR(meson_dw_hdmi->hdmi);
>>
>> -meson_dw_hdmi_init(meson_dw_hdmi);
>> -
>> next_bridge = of_drm_find_bridge(pdev->dev.of_node);
>> if (next_bridge)
>> drm_bridge_attach(encoder, next_bridge,
>>
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/4] drm: dw-hdmi-i2s: Use fixed id for codec device
On Wed 18 Sep 2019 at 10:24, Cheng-Yi Chiang wrote:
> The problem of using auto ID is that the device name will be like
> hdmi-audio-codec..auto.
>
> The number might be changed when there are other platform devices being
> created before hdmi-audio-codec device.
> Use a fixed name so machine driver can set codec name on the DAI link.
>
> Using the fixed name should be fine because there will only be one
> hdmi-audio-codec device.
While this is true all platforms we know of (I suppose), It might not be
the case later on. I wonder if making such assumption is really
desirable in a code which is used by quite a few different platforms.
Instead of trying to predict what the device name will be, can't you just
query it in your machine driver ? Using a device tree phandle maybe ?
It is quite usual to set the dai links this way, "simple-card" is a good
example of this.
>
> Fix the codec name in rockchip rk3288_hdmi_analog machine driver.
>
> Signed-off-by: Cheng-Yi Chiang
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 2 +-
> sound/soc/rockchip/rk3288_hdmi_analog.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> index d7e65c869415..86bd482b9f94 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> @@ -193,7 +193,7 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev)
>
> memset(, 0, sizeof(pdevinfo));
> pdevinfo.parent = pdev->dev.parent;
> - pdevinfo.id = PLATFORM_DEVID_AUTO;
> + pdevinfo.id = PLATFORM_DEVID_NONE;
> pdevinfo.name = HDMI_CODEC_DRV_NAME;
> pdevinfo.data =
> pdevinfo.size_data = sizeof(pdata);
> diff --git a/sound/soc/rockchip/rk3288_hdmi_analog.c
> b/sound/soc/rockchip/rk3288_hdmi_analog.c
> index 767700c34ee2..8286025a8747 100644
> --- a/sound/soc/rockchip/rk3288_hdmi_analog.c
> +++ b/sound/soc/rockchip/rk3288_hdmi_analog.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -142,7 +143,7 @@ static const struct snd_soc_ops rk_ops = {
> SND_SOC_DAILINK_DEFS(audio,
> DAILINK_COMP_ARRAY(COMP_EMPTY()),
> DAILINK_COMP_ARRAY(COMP_CODEC(NULL, NULL),
> -COMP_CODEC("hdmi-audio-codec.2.auto", "i2s-hifi")),
> +COMP_CODEC(HDMI_CODEC_DRV_NAME, "i2s-hifi")),
> DAILINK_COMP_ARRAY(COMP_EMPTY()));
>
> static struct snd_soc_dai_link rk_dailink = {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 5/8] drm/bridge: dw-hdmi-i2s: set the channel allocation
setup the channel allocation provided by the generic hdmi-codec driver Reviewed-by: Jonas Karlman Signed-off-by: Jerome Brunet --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c index caf8aed78fea..0864dee8d180 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c @@ -85,6 +85,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data, dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate); dw_hdmi_set_channel_count(hdmi, hparms->channels); + dw_hdmi_set_channel_allocation(hdmi, hparms->cea.channel_allocation); hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS); hdmi_write(audio, conf0, HDMI_AUD_CONF0); -- 2.21.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 6/8] drm/bridge: dw-hdmi-i2s: reset audio fifo before applying new params
When changing the audio hw params, reset the audio fifo to make sure
any old remaining data is flushed.
The databook mentions that such reset should be followed by a reset of
the i2s block to make sure the samples stay aligned
Reviewed-by: Jonas Karlman
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 6 --
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 0864dee8d180..41bee0099578 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -49,6 +49,10 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
return -EINVAL;
}
+ /* Reset the FIFOs before applying new params */
+ hdmi_write(audio, HDMI_AUD_CONF0_SW_RESET, HDMI_AUD_CONF0);
+ hdmi_write(audio, (u8)~HDMI_MC_SWRSTZ_I2SSWRST_REQ, HDMI_MC_SWRSTZ);
+
inputclkfs = HDMI_AUD_INPUTCLKFS_64FS;
conf0 = HDMI_AUD_CONF0_I2S_ALL_ENABLE;
@@ -102,8 +106,6 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev,
void *data)
struct dw_hdmi *hdmi = audio->hdmi;
dw_hdmi_audio_disable(hdmi);
-
- hdmi_write(audio, HDMI_AUD_CONF0_SW_RESET, HDMI_AUD_CONF0);
}
static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 091d7c28aa17..a272fa393ae6 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -940,6 +940,7 @@ enum {
HDMI_MC_CLKDIS_PIXELCLK_DISABLE = 0x1,
/* MC_SWRSTZ field values */
+ HDMI_MC_SWRSTZ_I2SSWRST_REQ = 0x08,
HDMI_MC_SWRSTZ_TMDSSWRST_REQ = 0x02,
/* MC_FLOWCTRL field values */
--
2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/8] drm/bridge: dw-hdmi: improve i2s support
On Mon 12 Aug 2019 at 14:19, Neil Armstrong wrote: > Hi, > > On 12/08/2019 14:07, Jerome Brunet wrote: >> The purpose of this patchset is to improve the support of the i2s >> interface of the synopsys hdmi controller. >> >> Once applied, the interface should support all the usual i2s bus formats, >> 8 channels playback and properly setup the channel number and allocation >> in the infoframes. >> >> Also, the dw-hdmi i2s interface will now provide the eld to the generic >> hdmi-codec so it can expose the related controls to user space. >> >> This work was inspired by Jonas Karlman's work, available here [0]. >> >> This was tested the Amlogic meson-g12a-sei510 platform. >> For this specific platform, which uses codec2codec links, there is a >> runtime dependency for patch 8 on this ASoC series [1]. >> >> Changes since v1 [2]: >> * Fix copy size in .get_eld() >> >> [0]: >> https://github.com/Kwiboo/linux-rockchip/commits/rockchip-5.2-for-libreelec-v5.2.3 >> [1]: https://lkml.kernel.org/r/20190725165949.29699-1-jbru...@baylibre.com >> [2]: https://lkml.kernel.org/r/20190805134102.24173-1-jbru...@baylibre.com >> >> Jerome Brunet (8): >> drm/bridge: dw-hdmi-i2s: support more i2s format >> drm/bridge: dw-hdmi: move audio channel setup out of ahb >> drm/bridge: dw-hdmi: set channel count in the infoframes >> drm/bridge: dw-hdmi-i2s: enable lpcm multi channels >> drm/bridge: dw-hdmi-i2s: set the channel allocation >> drm/bridge: dw-hdmi-i2s: reset audio fifo before applying new params >> drm/bridge: dw-hdmi-i2s: enable only the required i2s lanes >> drm/bridge: dw-hdmi-i2s: add .get_eld support > > Reviewed-by: Neil Armstrong > > Jonas, is patch 8 ok for you now ? If yes I'll apply them to > drm-misc-next. Please don't ! I did not pick the right change and what I just sent is just crazy, give me a second to resend > > Neil > >> >> .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 20 ++- >> .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 + >> .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 60 +-- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 13 +++- >> include/drm/bridge/dw_hdmi.h | 2 + >> 6 files changed, 108 insertions(+), 25 deletions(-) >> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/8] drm/bridge: dw-hdmi: improve i2s support
The purpose of this patchset is to improve the support of the i2s interface of the synopsys hdmi controller. Once applied, the interface should support all the usual i2s bus formats, 8 channels playback and properly setup the channel number and allocation in the infoframes. Also, the dw-hdmi i2s interface will now provide the eld to the generic hdmi-codec so it can expose the related controls to user space. This work was inspired by Jonas Karlman's work, available here [0]. This was tested the Amlogic meson-g12a-sei510 platform. For this specific platform, which uses codec2codec links, there is a runtime dependency for patch 8 on this ASoC series [1]. Changes since v1 [2]: * Fix copy size in .get_eld() [0]: https://github.com/Kwiboo/linux-rockchip/commits/rockchip-5.2-for-libreelec-v5.2.3 [1]: https://lkml.kernel.org/r/20190725165949.29699-1-jbru...@baylibre.com [2]: https://lkml.kernel.org/r/20190805134102.24173-1-jbru...@baylibre.com Jerome Brunet (8): drm/bridge: dw-hdmi-i2s: support more i2s format drm/bridge: dw-hdmi: move audio channel setup out of ahb drm/bridge: dw-hdmi: set channel count in the infoframes drm/bridge: dw-hdmi-i2s: enable lpcm multi channels drm/bridge: dw-hdmi-i2s: set the channel allocation drm/bridge: dw-hdmi-i2s: reset audio fifo before applying new params drm/bridge: dw-hdmi-i2s: enable only the required i2s lanes drm/bridge: dw-hdmi-i2s: add .get_eld support .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 20 ++- .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 + .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 60 +-- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 13 +++- include/drm/bridge/dw_hdmi.h | 2 + 6 files changed, 108 insertions(+), 25 deletions(-) -- 2.21.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND PATCH v2 8/8] drm/bridge: dw-hdmi-i2s: add .get_eld support
Provide the eld to the generic hdmi-codec driver.
This will let the driver enforce the maximum channel number and set the
channel allocation depending on the hdmi sink.
Cc: Jonas Karlman
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 +
drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 11 +++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 +
3 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
index 63b5756f463b..cb07dc0da5a7 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
@@ -14,6 +14,7 @@ struct dw_hdmi_audio_data {
struct dw_hdmi_i2s_audio_data {
struct dw_hdmi *hdmi;
+ u8 *eld;
void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
u8 (*read)(struct dw_hdmi *hdmi, int offset);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index b8ece9c1ba2c..1d15cf9b6821 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -10,6 +10,7 @@
#include
#include
+#include
#include
@@ -121,6 +122,15 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev,
void *data)
dw_hdmi_audio_disable(hdmi);
}
+static int dw_hdmi_i2s_get_eld(struct device *dev, void *data, uint8_t *buf,
+ size_t len)
+{
+ struct dw_hdmi_i2s_audio_data *audio = data;
+
+ memcpy(buf, audio->eld, min_t(size_t, MAX_ELD_BYTES, len));
+ return 0;
+}
+
static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
struct device_node *endpoint)
{
@@ -144,6 +154,7 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component
*component,
static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
.hw_params = dw_hdmi_i2s_hw_params,
.audio_shutdown = dw_hdmi_i2s_audio_shutdown,
+ .get_eld= dw_hdmi_i2s_get_eld,
.get_dai_id = dw_hdmi_i2s_get_dai_id,
};
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index bed4bb017afd..8df69c9dbfad 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2797,6 +2797,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
struct dw_hdmi_i2s_audio_data audio;
audio.hdmi = hdmi;
+ audio.eld = hdmi->connector.eld;
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
--
2.21.0
[PATCH v2 1/8] drm/bridge: dw-hdmi-i2s: support more i2s format
The dw-hdmi-i2s supports more formats than just regular i2s.
Add support for left justified, right justified and dsp modes
A and B.
Reviewed-by: Jonas Karlman
Signed-off-by: Jerome Brunet
---
.../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 26 ---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 6 +++--
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 5cbb71a866d5..2b624cff541d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -44,9 +44,8 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
u8 inputclkfs = 0;
/* it cares I2S only */
- if ((fmt->fmt != HDMI_I2S) ||
- (fmt->bit_clk_master | fmt->frame_clk_master)) {
- dev_err(dev, "unsupported format/settings\n");
+ if (fmt->bit_clk_master | fmt->frame_clk_master) {
+ dev_err(dev, "unsupported clock settings\n");
return -EINVAL;
}
@@ -63,6 +62,27 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
break;
}
+ switch (fmt->fmt) {
+ case HDMI_I2S:
+ conf1 |= HDMI_AUD_CONF1_MODE_I2S;
+ break;
+ case HDMI_RIGHT_J:
+ conf1 |= HDMI_AUD_CONF1_MODE_RIGHT_J;
+ break;
+ case HDMI_LEFT_J:
+ conf1 |= HDMI_AUD_CONF1_MODE_LEFT_J;
+ break;
+ case HDMI_DSP_A:
+ conf1 |= HDMI_AUD_CONF1_MODE_BURST_1;
+ break;
+ case HDMI_DSP_B:
+ conf1 |= HDMI_AUD_CONF1_MODE_BURST_2;
+ break;
+ default:
+ dev_err(dev, "unsupported format\n");
+ return -EINVAL;
+ }
+
dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 4e3ec09d3ca4..091d7c28aa17 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -869,8 +869,10 @@ enum {
/* AUD_CONF1 field values */
HDMI_AUD_CONF1_MODE_I2S = 0x00,
- HDMI_AUD_CONF1_MODE_RIGHT_J = 0x02,
- HDMI_AUD_CONF1_MODE_LEFT_J = 0x04,
+ HDMI_AUD_CONF1_MODE_RIGHT_J = 0x20,
+ HDMI_AUD_CONF1_MODE_LEFT_J = 0x40,
+ HDMI_AUD_CONF1_MODE_BURST_1 = 0x60,
+ HDMI_AUD_CONF1_MODE_BURST_2 = 0x80,
HDMI_AUD_CONF1_WIDTH_16 = 0x10,
HDMI_AUD_CONF1_WIDTH_24 = 0x18,
--
2.21.0
[PATCH v2 3/8] drm/bridge: dw-hdmi: set channel count in the infoframes
Set the number of channel in the infoframes Reviewed-by: Jonas Karlman Signed-off-by: Jerome Brunet --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index be6d6819bef4..bed4bb017afd 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -663,6 +663,10 @@ void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt) hdmi_modb(hdmi, layout, HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_MASK, HDMI_FC_AUDSCONF); + /* Set the audio infoframes channel count */ + hdmi_modb(hdmi, (cnt - 1) << HDMI_FC_AUDICONF0_CC_OFFSET, + HDMI_FC_AUDICONF0_CC_MASK, HDMI_FC_AUDICONF0); + mutex_unlock(>audio_mutex); } EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_count); -- 2.21.0
[PATCH v2 8/8] drm/bridge: dw-hdmi-i2s: add .get_eld support
Provide the eld to the generic hdmi-codec driver.
This will let the driver enforce the maximum channel number and set the
channel allocation depending on the hdmi sink.
Cc: Jonas Karlman
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 +
drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 11 +++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 +
3 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
index 63b5756f463b..cb07dc0da5a7 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
@@ -14,6 +14,7 @@ struct dw_hdmi_audio_data {
struct dw_hdmi_i2s_audio_data {
struct dw_hdmi *hdmi;
+ u8 *eld;
void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
u8 (*read)(struct dw_hdmi *hdmi, int offset);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index b8ece9c1ba2c..62e737b81462 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -10,6 +10,7 @@
#include
#include
+#include
#include
@@ -121,6 +122,15 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev,
void *data)
dw_hdmi_audio_disable(hdmi);
}
+static int dw_hdmi_i2s_get_eld(struct device *dev, void *data, uint8_t *buf,
+ size_t len)
+{
+ struct dw_hdmi_i2s_audio_data *audio = data;
+
+ memcpy(buf, audio->eld, min(sizeof(MAX_ELD_BYTES), len));
+ return 0;
+}
+
static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
struct device_node *endpoint)
{
@@ -144,6 +154,7 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component
*component,
static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
.hw_params = dw_hdmi_i2s_hw_params,
.audio_shutdown = dw_hdmi_i2s_audio_shutdown,
+ .get_eld= dw_hdmi_i2s_get_eld,
.get_dai_id = dw_hdmi_i2s_get_dai_id,
};
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index bed4bb017afd..8df69c9dbfad 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2797,6 +2797,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
struct dw_hdmi_i2s_audio_data audio;
audio.hdmi = hdmi;
+ audio.eld = hdmi->connector.eld;
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
--
2.21.0
[PATCH v2 7/8] drm/bridge: dw-hdmi-i2s: enable only the required i2s lanes
Enable the i2s lanes depending on the number of channel in the stream
Reviewed-by: Jonas Karlman
Signed-off-by: Jerome Brunet
---
.../gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 15 ++-
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 6 +-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 41bee0099578..b8ece9c1ba2c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -54,7 +54,20 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
hdmi_write(audio, (u8)~HDMI_MC_SWRSTZ_I2SSWRST_REQ, HDMI_MC_SWRSTZ);
inputclkfs = HDMI_AUD_INPUTCLKFS_64FS;
- conf0 = HDMI_AUD_CONF0_I2S_ALL_ENABLE;
+ conf0 = (HDMI_AUD_CONF0_I2S_SELECT | HDMI_AUD_CONF0_I2S_EN0);
+
+ /* Enable the required i2s lanes */
+ switch (hparms->channels) {
+ case 7 ... 8:
+ conf0 |= HDMI_AUD_CONF0_I2S_EN3;
+ /* Fall-thru */
+ case 5 ... 6:
+ conf0 |= HDMI_AUD_CONF0_I2S_EN2;
+ /* Fall-thru */
+ case 3 ... 4:
+ conf0 |= HDMI_AUD_CONF0_I2S_EN1;
+ /* Fall-thru */
+ }
switch (hparms->sample_width) {
case 16:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index a272fa393ae6..6988f12d89d9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -865,7 +865,11 @@ enum {
/* AUD_CONF0 field values */
HDMI_AUD_CONF0_SW_RESET = 0x80,
- HDMI_AUD_CONF0_I2S_ALL_ENABLE = 0x2F,
+ HDMI_AUD_CONF0_I2S_SELECT = 0x20,
+ HDMI_AUD_CONF0_I2S_EN3 = 0x08,
+ HDMI_AUD_CONF0_I2S_EN2 = 0x04,
+ HDMI_AUD_CONF0_I2S_EN1 = 0x02,
+ HDMI_AUD_CONF0_I2S_EN0 = 0x01,
/* AUD_CONF1 field values */
HDMI_AUD_CONF1_MODE_I2S = 0x00,
--
2.21.0
[PATCH v2 2/8] drm/bridge: dw-hdmi: move audio channel setup out of ahb
Part of the channel count setup done in dw-hdmi ahb should
actually be done whatever the interface providing the data.
Reviewed-by: Jonas Karlman
Let's move it to dw-hdmi driver instead.
Signed-off-by: Jerome Brunet
---
.../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 20 +++-
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 32 +++
include/drm/bridge/dw_hdmi.h | 2 ++
3 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index a494186ae6ce..2b7539701b42 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -63,10 +63,6 @@ enum {
HDMI_REVISION_ID = 0x0001,
HDMI_IH_AHBDMAAUD_STAT0 = 0x0109,
HDMI_IH_MUTE_AHBDMAAUD_STAT0 = 0x0189,
- HDMI_FC_AUDICONF2 = 0x1027,
- HDMI_FC_AUDSCONF = 0x1063,
- HDMI_FC_AUDSCONF_LAYOUT1 = 1 << 0,
- HDMI_FC_AUDSCONF_LAYOUT0 = 0 << 0,
HDMI_AHB_DMA_CONF0 = 0x3600,
HDMI_AHB_DMA_START = 0x3601,
HDMI_AHB_DMA_STOP = 0x3602,
@@ -403,7 +399,7 @@ static int dw_hdmi_prepare(struct snd_pcm_substream
*substream)
{
struct snd_pcm_runtime *runtime = substream->runtime;
struct snd_dw_hdmi *dw = substream->private_data;
- u8 threshold, conf0, conf1, layout, ca;
+ u8 threshold, conf0, conf1, ca;
/* Setup as per 3.0.5 FSL 4.1.0 BSP */
switch (dw->revision) {
@@ -434,20 +430,12 @@ static int dw_hdmi_prepare(struct snd_pcm_substream
*substream)
conf1 = default_hdmi_channel_config[runtime->channels - 2].conf1;
ca = default_hdmi_channel_config[runtime->channels - 2].ca;
- /*
-* For >2 channel PCM audio, we need to select layout 1
-* and set an appropriate channel map.
-*/
- if (runtime->channels > 2)
- layout = HDMI_FC_AUDSCONF_LAYOUT1;
- else
- layout = HDMI_FC_AUDSCONF_LAYOUT0;
-
writeb_relaxed(threshold, dw->data.base + HDMI_AHB_DMA_THRSLD);
writeb_relaxed(conf0, dw->data.base + HDMI_AHB_DMA_CONF0);
writeb_relaxed(conf1, dw->data.base + HDMI_AHB_DMA_CONF1);
- writeb_relaxed(layout, dw->data.base + HDMI_FC_AUDSCONF);
- writeb_relaxed(ca, dw->data.base + HDMI_FC_AUDICONF2);
+
+ dw_hdmi_set_channel_count(dw->data.hdmi, runtime->channels);
+ dw_hdmi_set_channel_allocation(dw->data.hdmi, ca);
switch (runtime->format) {
case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 218a7b2308f7..be6d6819bef4 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -645,6 +645,38 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt)
+{
+ u8 layout;
+
+ mutex_lock(>audio_mutex);
+
+ /*
+* For >2 channel PCM audio, we need to select layout 1
+* and set an appropriate channel map.
+*/
+ if (cnt > 2)
+ layout = HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_LAYOUT1;
+ else
+ layout = HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_LAYOUT0;
+
+ hdmi_modb(hdmi, layout, HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_MASK,
+ HDMI_FC_AUDSCONF);
+
+ mutex_unlock(>audio_mutex);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_count);
+
+void dw_hdmi_set_channel_allocation(struct dw_hdmi *hdmi, unsigned int ca)
+{
+ mutex_lock(>audio_mutex);
+
+ hdmi_writeb(hdmi, ca, HDMI_FC_AUDICONF2);
+
+ mutex_unlock(>audio_mutex);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_allocation);
+
static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
{
if (enable)
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index c402364aec0d..cf528c289857 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -155,6 +155,8 @@ void dw_hdmi_resume(struct dw_hdmi *hdmi);
void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
+void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt);
+void dw_hdmi_set_channel_allocation(struct dw_hdmi *hdmi, unsigned int ca);
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi);
--
2.21.0
[PATCH v2 4/8] drm/bridge: dw-hdmi-i2s: enable lpcm multi channels
Properly setup the channel count and layout in dw-hdmi i2s driver so we are not limited to 2 channels. Also correct the maximum channel reported by the DAI from 6 to 8 ch Reviewed-by: Jonas Karlman Signed-off-by: Jerome Brunet --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c index 2b624cff541d..caf8aed78fea 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c @@ -84,6 +84,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data, } dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate); + dw_hdmi_set_channel_count(hdmi, hparms->channels); hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS); hdmi_write(audio, conf0, HDMI_AUD_CONF0); @@ -139,7 +140,7 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev) pdata.ops = _hdmi_i2s_ops; pdata.i2s = 1; - pdata.max_i2s_channels = 6; + pdata.max_i2s_channels = 8; pdata.data = audio; memset(, 0, sizeof(pdevinfo)); -- 2.21.0
Re: [PATCH 8/8] drm/bridge: dw-hdmi-i2s: add .get_eld support
On Wed 07 Aug 2019 at 14:57, Jonas Karlman wrote:
> On 2019-08-05 15:41, Jerome Brunet wrote:
>> Provide the eld to the generic hdmi-codec driver.
>> This will let the driver enforce the maximum channel number and set the
>> channel allocation depending on the hdmi sink.
>>
>> Cc: Jonas Karlman
>> Signed-off-by: Jerome Brunet
>> ---
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 +
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++
>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 +
>> 3 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> index 63b5756f463b..cb07dc0da5a7 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
>> @@ -14,6 +14,7 @@ struct dw_hdmi_audio_data {
>>
>> struct dw_hdmi_i2s_audio_data {
>> struct dw_hdmi *hdmi;
>> +u8 *eld;
>>
>> void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
>> u8 (*read)(struct dw_hdmi *hdmi, int offset);
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> index b8ece9c1ba2c..14d499b344c0 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
>> @@ -121,6 +121,15 @@ static void dw_hdmi_i2s_audio_shutdown(struct device
>> *dev, void *data)
>> dw_hdmi_audio_disable(hdmi);
>> }
>>
>> +static int dw_hdmi_i2s_get_eld(struct device *dev, void *data, uint8_t *buf,
>> + size_t len)
>> +{
>> +struct dw_hdmi_i2s_audio_data *audio = data;
>> +
>> +memcpy(buf, audio->eld, min(sizeof(audio->eld), len));
>
> Above sizeof does not work as intended, sizeof(audio->eld) is probably the
> size of a pointer and not MAX_ELD_BYTES (128),
> resulting in only part of the ELD gets copied to buf.
Silly ... thanks for pointing this out. I'll respin
>
>
> Thanks for reworking dw-hdmi multi channel lpcm support!, this works on
> Rockchip for most parts.
> Without the [1] patch (reset cts+n after clock is enabled) audio sometime
> stay silent when switching between e.g. 2ch 44.1khz and 6ch 48khz tracks.
> It is not fully clear to me why reset cts+n helps, if that have
> negative affects on other platforms or if there is another way to
> solve loosing audio.
I did not see that issue my self but I guess could propose this change ?
>
> I also have a small issue with the channel allocation being selected by
> hdmi-codec, my soundbar reports LPCM 6.1ch instead of LPCM 7.1ch when I play
> a 7.1ch speaker test clip.
> I have a hdmi-codec patch to fix selection of channel allocation in
> queue.
Yes, I know there is still a few problems around that and stale eld.
But those problem are not really coming from the i2s interface of the
dw-hdmi itself and should be dealt with separatly.
This is just the beginning ;)
>
>
> For patch 1-7:
>
> Reviewed-by: Jonas Karlman
>
>
> [1]
> https://github.com/Kwiboo/linux-rockchip/commit/c0839e874f843ad173ddde958303d6878394ef92
>
> Regards,
> Jonas
>
>> +return 0;
>> +}
>> +
>> static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
>>struct device_node *endpoint)
>> {
>> @@ -144,6 +153,7 @@ static int dw_hdmi_i2s_get_dai_id(struct
>> snd_soc_component *component,
>> static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
>> .hw_params = dw_hdmi_i2s_hw_params,
>> .audio_shutdown = dw_hdmi_i2s_audio_shutdown,
>> +.get_eld= dw_hdmi_i2s_get_eld,
>> .get_dai_id = dw_hdmi_i2s_get_dai_id,
>> };
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index bed4bb017afd..8df69c9dbfad 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -2797,6 +2797,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
>> struct dw_hdmi_i2s_audio_data audio;
>>
>> audio.hdmi = hdmi;
>> +audio.eld = hdmi->connector.eld;
>> audio.write = hdmi_writeb;
>> audio.read = hdmi_readb;
>> hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
[PATCH 4/8] drm/bridge: dw-hdmi-i2s: enable lpcm multi channels
Properly setup the channel count and layout in dw-hdmi i2s driver so we are not limited to 2 channels. Also correct the maximum channel reported by the DAI from 6 to 8 ch Cc: Jonas Karlman Signed-off-by: Jerome Brunet --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c index 2b624cff541d..caf8aed78fea 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c @@ -84,6 +84,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data, } dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate); + dw_hdmi_set_channel_count(hdmi, hparms->channels); hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS); hdmi_write(audio, conf0, HDMI_AUD_CONF0); @@ -139,7 +140,7 @@ static int snd_dw_hdmi_probe(struct platform_device *pdev) pdata.ops = _hdmi_i2s_ops; pdata.i2s = 1; - pdata.max_i2s_channels = 6; + pdata.max_i2s_channels = 8; pdata.data = audio; memset(, 0, sizeof(pdevinfo)); -- 2.21.0
[PATCH 0/8] drm/bridge: dw-hdmi: improve i2s support
The purpose of this patchset is to improve the support of the i2s interface of the synopsys hdmi controller. Once applied, the interface should support all the usual i2s bus formats, 8 channels playback and properly setup the channel number and allocation in the infoframes. Also, the dw-hdmi i2s interface will now provide the eld to the generic hdmi-codec so it can expose the related controls to user space. This work was inspired by Jonas Karlman's work, available here [0]. This was tested the Amlogic meson-g12a-sei510 platform. For this specific platform, which uses codec2codec links, there is a runtime dependency for patch 8 on this ASoC series [1]. [0]: https://github.com/Kwiboo/linux-rockchip/commits/rockchip-5.2-for-libreelec-v5.2.3 [1]: https://lkml.kernel.org/r/20190725165949.29699-1-jbru...@baylibre.com Jerome Brunet (8): drm/bridge: dw-hdmi-i2s: support more i2s format drm/bridge: dw-hdmi: move audio channel setup out of ahb drm/bridge: dw-hdmi: set channel count in the infoframes drm/bridge: dw-hdmi-i2s: enable lpcm multi channels drm/bridge: dw-hdmi-i2s: set the channel allocation drm/bridge: dw-hdmi-i2s: reset audio fifo before applying new params drm/bridge: dw-hdmi-i2s: enable only the required i2s lanes drm/bridge: dw-hdmi-i2s: add .get_eld support .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 20 ++- .../gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 + .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 59 +-- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 37 drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 13 +++- include/drm/bridge/dw_hdmi.h | 2 + 6 files changed, 107 insertions(+), 25 deletions(-) -- 2.21.0
[PATCH 6/8] drm/bridge: dw-hdmi-i2s: reset audio fifo before applying new params
When changing the audio hw params, reset the audio fifo to make sure
any old remaining data is flushed.
The databook mentions that such reset should be followed by a reset of
the i2s block to make sure the samples stay aligned
Cc: Jonas Karlman
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 6 --
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 0864dee8d180..41bee0099578 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -49,6 +49,10 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
return -EINVAL;
}
+ /* Reset the FIFOs before applying new params */
+ hdmi_write(audio, HDMI_AUD_CONF0_SW_RESET, HDMI_AUD_CONF0);
+ hdmi_write(audio, (u8)~HDMI_MC_SWRSTZ_I2SSWRST_REQ, HDMI_MC_SWRSTZ);
+
inputclkfs = HDMI_AUD_INPUTCLKFS_64FS;
conf0 = HDMI_AUD_CONF0_I2S_ALL_ENABLE;
@@ -102,8 +106,6 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev,
void *data)
struct dw_hdmi *hdmi = audio->hdmi;
dw_hdmi_audio_disable(hdmi);
-
- hdmi_write(audio, HDMI_AUD_CONF0_SW_RESET, HDMI_AUD_CONF0);
}
static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 091d7c28aa17..a272fa393ae6 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -940,6 +940,7 @@ enum {
HDMI_MC_CLKDIS_PIXELCLK_DISABLE = 0x1,
/* MC_SWRSTZ field values */
+ HDMI_MC_SWRSTZ_I2SSWRST_REQ = 0x08,
HDMI_MC_SWRSTZ_TMDSSWRST_REQ = 0x02,
/* MC_FLOWCTRL field values */
--
2.21.0
[PATCH 2/8] drm/bridge: dw-hdmi: move audio channel setup out of ahb
Part of the channel count setup done in dw-hdmi ahb should
actually be done whatever the interface providing the data.
Let's move it to dw-hdmi driver instead.
Signed-off-by: Jerome Brunet
---
.../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 20 +++-
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 32 +++
include/drm/bridge/dw_hdmi.h | 2 ++
3 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index a494186ae6ce..2b7539701b42 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -63,10 +63,6 @@ enum {
HDMI_REVISION_ID = 0x0001,
HDMI_IH_AHBDMAAUD_STAT0 = 0x0109,
HDMI_IH_MUTE_AHBDMAAUD_STAT0 = 0x0189,
- HDMI_FC_AUDICONF2 = 0x1027,
- HDMI_FC_AUDSCONF = 0x1063,
- HDMI_FC_AUDSCONF_LAYOUT1 = 1 << 0,
- HDMI_FC_AUDSCONF_LAYOUT0 = 0 << 0,
HDMI_AHB_DMA_CONF0 = 0x3600,
HDMI_AHB_DMA_START = 0x3601,
HDMI_AHB_DMA_STOP = 0x3602,
@@ -403,7 +399,7 @@ static int dw_hdmi_prepare(struct snd_pcm_substream
*substream)
{
struct snd_pcm_runtime *runtime = substream->runtime;
struct snd_dw_hdmi *dw = substream->private_data;
- u8 threshold, conf0, conf1, layout, ca;
+ u8 threshold, conf0, conf1, ca;
/* Setup as per 3.0.5 FSL 4.1.0 BSP */
switch (dw->revision) {
@@ -434,20 +430,12 @@ static int dw_hdmi_prepare(struct snd_pcm_substream
*substream)
conf1 = default_hdmi_channel_config[runtime->channels - 2].conf1;
ca = default_hdmi_channel_config[runtime->channels - 2].ca;
- /*
-* For >2 channel PCM audio, we need to select layout 1
-* and set an appropriate channel map.
-*/
- if (runtime->channels > 2)
- layout = HDMI_FC_AUDSCONF_LAYOUT1;
- else
- layout = HDMI_FC_AUDSCONF_LAYOUT0;
-
writeb_relaxed(threshold, dw->data.base + HDMI_AHB_DMA_THRSLD);
writeb_relaxed(conf0, dw->data.base + HDMI_AHB_DMA_CONF0);
writeb_relaxed(conf1, dw->data.base + HDMI_AHB_DMA_CONF1);
- writeb_relaxed(layout, dw->data.base + HDMI_FC_AUDSCONF);
- writeb_relaxed(ca, dw->data.base + HDMI_FC_AUDICONF2);
+
+ dw_hdmi_set_channel_count(dw->data.hdmi, runtime->channels);
+ dw_hdmi_set_channel_allocation(dw->data.hdmi, ca);
switch (runtime->format) {
case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 218a7b2308f7..be6d6819bef4 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -645,6 +645,38 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi,
unsigned int rate)
}
EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
+void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt)
+{
+ u8 layout;
+
+ mutex_lock(>audio_mutex);
+
+ /*
+* For >2 channel PCM audio, we need to select layout 1
+* and set an appropriate channel map.
+*/
+ if (cnt > 2)
+ layout = HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_LAYOUT1;
+ else
+ layout = HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_LAYOUT0;
+
+ hdmi_modb(hdmi, layout, HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_MASK,
+ HDMI_FC_AUDSCONF);
+
+ mutex_unlock(>audio_mutex);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_count);
+
+void dw_hdmi_set_channel_allocation(struct dw_hdmi *hdmi, unsigned int ca)
+{
+ mutex_lock(>audio_mutex);
+
+ hdmi_writeb(hdmi, ca, HDMI_FC_AUDICONF2);
+
+ mutex_unlock(>audio_mutex);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_allocation);
+
static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
{
if (enable)
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index c402364aec0d..cf528c289857 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -155,6 +155,8 @@ void dw_hdmi_resume(struct dw_hdmi *hdmi);
void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
+void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt);
+void dw_hdmi_set_channel_allocation(struct dw_hdmi *hdmi, unsigned int ca);
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi);
--
2.21.0
[PATCH 5/8] drm/bridge: dw-hdmi-i2s: set the channel allocation
setup the channel allocation provided by the generic hdmi-codec driver Cc: Jonas Karlman Signed-off-by: Jerome Brunet --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c index caf8aed78fea..0864dee8d180 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c @@ -85,6 +85,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data, dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate); dw_hdmi_set_channel_count(hdmi, hparms->channels); + dw_hdmi_set_channel_allocation(hdmi, hparms->cea.channel_allocation); hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS); hdmi_write(audio, conf0, HDMI_AUD_CONF0); -- 2.21.0
[PATCH 3/8] drm/bridge: dw-hdmi: set channel count in the infoframes
Set the number of channel in the infoframes Cc: Jonas Karlman Signed-off-by: Jerome Brunet --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index be6d6819bef4..bed4bb017afd 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -663,6 +663,10 @@ void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt) hdmi_modb(hdmi, layout, HDMI_FC_AUDSCONF_AUD_PACKET_LAYOUT_MASK, HDMI_FC_AUDSCONF); + /* Set the audio infoframes channel count */ + hdmi_modb(hdmi, (cnt - 1) << HDMI_FC_AUDICONF0_CC_OFFSET, + HDMI_FC_AUDICONF0_CC_MASK, HDMI_FC_AUDICONF0); + mutex_unlock(>audio_mutex); } EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_count); -- 2.21.0
[PATCH 8/8] drm/bridge: dw-hdmi-i2s: add .get_eld support
Provide the eld to the generic hdmi-codec driver.
This will let the driver enforce the maximum channel number and set the
channel allocation depending on the hdmi sink.
Cc: Jonas Karlman
Signed-off-by: Jerome Brunet
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h | 1 +
drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 10 ++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 +
3 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
index 63b5756f463b..cb07dc0da5a7 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-audio.h
@@ -14,6 +14,7 @@ struct dw_hdmi_audio_data {
struct dw_hdmi_i2s_audio_data {
struct dw_hdmi *hdmi;
+ u8 *eld;
void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
u8 (*read)(struct dw_hdmi *hdmi, int offset);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index b8ece9c1ba2c..14d499b344c0 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -121,6 +121,15 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev,
void *data)
dw_hdmi_audio_disable(hdmi);
}
+static int dw_hdmi_i2s_get_eld(struct device *dev, void *data, uint8_t *buf,
+ size_t len)
+{
+ struct dw_hdmi_i2s_audio_data *audio = data;
+
+ memcpy(buf, audio->eld, min(sizeof(audio->eld), len));
+ return 0;
+}
+
static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component *component,
struct device_node *endpoint)
{
@@ -144,6 +153,7 @@ static int dw_hdmi_i2s_get_dai_id(struct snd_soc_component
*component,
static struct hdmi_codec_ops dw_hdmi_i2s_ops = {
.hw_params = dw_hdmi_i2s_hw_params,
.audio_shutdown = dw_hdmi_i2s_audio_shutdown,
+ .get_eld= dw_hdmi_i2s_get_eld,
.get_dai_id = dw_hdmi_i2s_get_dai_id,
};
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index bed4bb017afd..8df69c9dbfad 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2797,6 +2797,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
struct dw_hdmi_i2s_audio_data audio;
audio.hdmi = hdmi;
+ audio.eld = hdmi->connector.eld;
audio.write = hdmi_writeb;
audio.read = hdmi_readb;
hdmi->enable_audio = dw_hdmi_i2s_audio_enable;
--
2.21.0
[PATCH 7/8] drm/bridge: dw-hdmi-i2s: enable only the required i2s lanes
Enable the i2s lanes depending on the number of channel in the stream
Cc: Jonas Karlman
Signed-off-by: Jerome Brunet
---
.../gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 15 ++-
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 6 +-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 41bee0099578..b8ece9c1ba2c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -54,7 +54,20 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
hdmi_write(audio, (u8)~HDMI_MC_SWRSTZ_I2SSWRST_REQ, HDMI_MC_SWRSTZ);
inputclkfs = HDMI_AUD_INPUTCLKFS_64FS;
- conf0 = HDMI_AUD_CONF0_I2S_ALL_ENABLE;
+ conf0 = (HDMI_AUD_CONF0_I2S_SELECT | HDMI_AUD_CONF0_I2S_EN0);
+
+ /* Enable the required i2s lanes */
+ switch (hparms->channels) {
+ case 7 ... 8:
+ conf0 |= HDMI_AUD_CONF0_I2S_EN3;
+ /* Fall-thru */
+ case 5 ... 6:
+ conf0 |= HDMI_AUD_CONF0_I2S_EN2;
+ /* Fall-thru */
+ case 3 ... 4:
+ conf0 |= HDMI_AUD_CONF0_I2S_EN1;
+ /* Fall-thru */
+ }
switch (hparms->sample_width) {
case 16:
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index a272fa393ae6..6988f12d89d9 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -865,7 +865,11 @@ enum {
/* AUD_CONF0 field values */
HDMI_AUD_CONF0_SW_RESET = 0x80,
- HDMI_AUD_CONF0_I2S_ALL_ENABLE = 0x2F,
+ HDMI_AUD_CONF0_I2S_SELECT = 0x20,
+ HDMI_AUD_CONF0_I2S_EN3 = 0x08,
+ HDMI_AUD_CONF0_I2S_EN2 = 0x04,
+ HDMI_AUD_CONF0_I2S_EN1 = 0x02,
+ HDMI_AUD_CONF0_I2S_EN0 = 0x01,
/* AUD_CONF1 field values */
HDMI_AUD_CONF1_MODE_I2S = 0x00,
--
2.21.0
[PATCH 1/8] drm/bridge: dw-hdmi-i2s: support more i2s format
The dw-hdmi-i2s supports more formats than just regular i2s.
Add support for left justified, right justified and dsp modes
A and B.
Signed-off-by: Jerome Brunet
---
.../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 26 ---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 6 +++--
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 5cbb71a866d5..2b624cff541d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -44,9 +44,8 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
u8 inputclkfs = 0;
/* it cares I2S only */
- if ((fmt->fmt != HDMI_I2S) ||
- (fmt->bit_clk_master | fmt->frame_clk_master)) {
- dev_err(dev, "unsupported format/settings\n");
+ if (fmt->bit_clk_master | fmt->frame_clk_master) {
+ dev_err(dev, "unsupported clock settings\n");
return -EINVAL;
}
@@ -63,6 +62,27 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
break;
}
+ switch (fmt->fmt) {
+ case HDMI_I2S:
+ conf1 |= HDMI_AUD_CONF1_MODE_I2S;
+ break;
+ case HDMI_RIGHT_J:
+ conf1 |= HDMI_AUD_CONF1_MODE_RIGHT_J;
+ break;
+ case HDMI_LEFT_J:
+ conf1 |= HDMI_AUD_CONF1_MODE_LEFT_J;
+ break;
+ case HDMI_DSP_A:
+ conf1 |= HDMI_AUD_CONF1_MODE_BURST_1;
+ break;
+ case HDMI_DSP_B:
+ conf1 |= HDMI_AUD_CONF1_MODE_BURST_2;
+ break;
+ default:
+ dev_err(dev, "unsupported format\n");
+ return -EINVAL;
+ }
+
dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 4e3ec09d3ca4..091d7c28aa17 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -869,8 +869,10 @@ enum {
/* AUD_CONF1 field values */
HDMI_AUD_CONF1_MODE_I2S = 0x00,
- HDMI_AUD_CONF1_MODE_RIGHT_J = 0x02,
- HDMI_AUD_CONF1_MODE_LEFT_J = 0x04,
+ HDMI_AUD_CONF1_MODE_RIGHT_J = 0x20,
+ HDMI_AUD_CONF1_MODE_LEFT_J = 0x40,
+ HDMI_AUD_CONF1_MODE_BURST_1 = 0x60,
+ HDMI_AUD_CONF1_MODE_BURST_2 = 0x80,
HDMI_AUD_CONF1_WIDTH_16 = 0x10,
HDMI_AUD_CONF1_WIDTH_24 = 0x18,
--
2.21.0
[PATCH] drm: dw-hdmi-i2s: support more i2s format
The dw-hdmi-i2s supports more formats than just regular i2s.
Add support for left justified, right justified and dsp modes
A and B.
Signed-off-by: Jerome Brunet
---
Tested on the Amlogic arm64 meson-g12a-sei510 with i2s, left_j, dsp_a
and dsp_b. right_j is not supported by this platform.
.../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 26 ---
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 6 +++--
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 5cbb71a866d5..2b624cff541d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -44,9 +44,8 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
u8 inputclkfs = 0;
/* it cares I2S only */
- if ((fmt->fmt != HDMI_I2S) ||
- (fmt->bit_clk_master | fmt->frame_clk_master)) {
- dev_err(dev, "unsupported format/settings\n");
+ if (fmt->bit_clk_master | fmt->frame_clk_master) {
+ dev_err(dev, "unsupported clock settings\n");
return -EINVAL;
}
@@ -63,6 +62,27 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void
*data,
break;
}
+ switch (fmt->fmt) {
+ case HDMI_I2S:
+ conf1 |= HDMI_AUD_CONF1_MODE_I2S;
+ break;
+ case HDMI_RIGHT_J:
+ conf1 |= HDMI_AUD_CONF1_MODE_RIGHT_J;
+ break;
+ case HDMI_LEFT_J:
+ conf1 |= HDMI_AUD_CONF1_MODE_LEFT_J;
+ break;
+ case HDMI_DSP_A:
+ conf1 |= HDMI_AUD_CONF1_MODE_BURST_1;
+ break;
+ case HDMI_DSP_B:
+ conf1 |= HDMI_AUD_CONF1_MODE_BURST_2;
+ break;
+ default:
+ dev_err(dev, "unsupported format\n");
+ return -EINVAL;
+ }
+
dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 4e3ec09d3ca4..091d7c28aa17 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -869,8 +869,10 @@ enum {
/* AUD_CONF1 field values */
HDMI_AUD_CONF1_MODE_I2S = 0x00,
- HDMI_AUD_CONF1_MODE_RIGHT_J = 0x02,
- HDMI_AUD_CONF1_MODE_LEFT_J = 0x04,
+ HDMI_AUD_CONF1_MODE_RIGHT_J = 0x20,
+ HDMI_AUD_CONF1_MODE_LEFT_J = 0x40,
+ HDMI_AUD_CONF1_MODE_BURST_1 = 0x60,
+ HDMI_AUD_CONF1_MODE_BURST_2 = 0x80,
HDMI_AUD_CONF1_WIDTH_16 = 0x10,
HDMI_AUD_CONF1_WIDTH_24 = 0x18,
--
2.20.1
[PATCH] drm/meson: imply dw-hdmi i2s audio for meson hdmi
Imply the i2s part of the Synopsys HDMI driver for Amlogic SoCs. This will enable the i2s part by default when meson hdmi driver is enable but let platforms not supported by the audio subsystem disable it if necessary. Signed-off-by: Jerome Brunet --- drivers/gpu/drm/meson/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig index c28b69f48555..a480e4a80bea 100644 --- a/drivers/gpu/drm/meson/Kconfig +++ b/drivers/gpu/drm/meson/Kconfig @@ -14,3 +14,4 @@ config DRM_MESON_DW_HDMI depends on DRM_MESON default y if DRM_MESON select DRM_DW_HDMI + imply DRM_DW_HDMI_I2S_AUDIO -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/11] drm/meson: Add G12A support for plane handling in CRTC driver
On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote:
> This patch adds support for the new OSD+VD Plane blending module
> in the CRTC code by adding the G12A code to manage the blending
> module and setting the right OSD1 & VD1 plane registers.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/meson/meson_crtc.c | 269 +++--
> drivers/gpu/drm/meson/meson_drv.h | 4 +
> 2 files changed, 221 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_crtc.c
> b/drivers/gpu/drm/meson/meson_crtc.c
> index 6d9311e254ef..5579f8ac3e3f 100644
> --- a/drivers/gpu/drm/meson/meson_crtc.c
> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> @@ -39,12 +39,17 @@
> #include "meson_viu.h"
> #include "meson_registers.h"
>
> +#define MESON_G12A_VIU_OFFSET0x5ec0
> +
> /* CRTC definition */
>
> struct meson_crtc {
> struct drm_crtc base;
> struct drm_pending_vblank_event *event;
> struct meson_drm *priv;
> + void (*enable_osd1)(struct meson_drm *priv);
> + void (*enable_vd1)(struct meson_drm *priv);
> + unsigned int viu_offset;
> };
> #define to_meson_crtc(x) container_of(x, struct meson_crtc, base)
>
> @@ -80,6 +85,44 @@ static const struct drm_crtc_funcs meson_crtc_funcs = {
>
> };
>
> +static void meson_g12a_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_crtc_state *old_state)
> +{
> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> + struct drm_crtc_state *crtc_state = crtc->state;
> + struct meson_drm *priv = meson_crtc->priv;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + if (!crtc_state) {
> + DRM_ERROR("Invalid crtc_state\n");
> + return;
> + }
> +
> + /* VD1 Preblend vertical start/end */
> + writel(FIELD_PREP(GENMASK(11, 0), 2303),
Could could define this mask somewhere
I don't know if the 2303 can be explained. I'd nice if it could
> +priv->io_base + _REG(VPP_PREBLEND_VD1_V_START_END));
> +
> + /* Setup Blender */
> + writel(crtc_state->mode.hdisplay |
> +crtc_state->mode.vdisplay << 16,
> +priv->io_base + _REG(VPP_POSTBLEND_H_SIZE));
> +
> + writel_relaxed(0 << 16 |
this 16 shift seems to be used for hdisplay or something. Could
define/document it a bit
> + (crtc_state->mode.hdisplay - 1),
> + priv->io_base + _REG(VPP_OSD1_BLD_H_SCOPE));
> + writel_relaxed(0 << 16 |
> + (crtc_state->mode.vdisplay - 1),
> + priv->io_base + _REG(VPP_OSD1_BLD_V_SCOPE));
> + writel_relaxed(crtc_state->mode.hdisplay << 16 |
> + crtc_state->mode.vdisplay,
> + priv->io_base + _REG(VPP_OUT_H_V_SIZE));
> +
> + drm_crtc_vblank_on(crtc);
> +
> + priv->viu.osd1_enabled = true;
> +}
> +
> static void meson_crtc_atomic_enable(struct drm_crtc *crtc,
>struct drm_crtc_state *old_state)
> {
> @@ -110,6 +153,31 @@ static void meson_crtc_atomic_enable(struct drm_crtc
> *crtc,
> priv->viu.osd1_enabled = true;
> }
>
> +static void meson_g12a_crtc_atomic_disable(struct drm_crtc *crtc,
> +struct drm_crtc_state *old_state)
> +{
> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> + struct meson_drm *priv = meson_crtc->priv;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + drm_crtc_vblank_off(crtc);
> +
> + priv->viu.osd1_enabled = false;
> + priv->viu.osd1_commit = false;
> +
> + priv->viu.vd1_enabled = false;
> + priv->viu.vd1_commit = false;
> +
> + if (crtc->state->event && !crtc->state->active) {
> + spin_lock_irq(>dev->event_lock);
> + drm_crtc_send_vblank_event(crtc, crtc->state->event);
> + spin_unlock_irq(>dev->event_lock);
> +
> + crtc->state->event = NULL;
> + }
> +}
> +
> static void meson_crtc_atomic_disable(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state)
> {
> @@ -173,6 +241,53 @@ static const struct drm_crtc_helper_funcs
> meson_crtc_helper_funcs = {
> .atomic_disable = meson_crtc_atomic_disable,
> };
>
> +static const struct drm_crtc_helper_funcs meson_g12a_crtc_helper_funcs = {
> + .atomic_begin = meson_crtc_atomic_begin,
> + .atomic_flush = meson_crtc_atomic_flush,
> + .atomic_enable = meson_g12a_crtc_atomic_enable,
> + .atomic_disable = meson_g12a_crtc_atomic_disable,
> +};
> +
> +static void meson_crtc_enable_osd1(struct meson_drm *priv)
> +{
> + writel_bits_relaxed(VPP_OSD1_POSTBLEND, VPP_OSD1_POSTBLEND,
> + priv->io_base + _REG(VPP_MISC));
> +}
> +
> +static void meson_g12a_crtc_enable_osd1(struct meson_drm *priv)
> +{
> + writel_relaxed(priv->viu.osd_blend_din0_scope_h,
> +priv->io_base +
> +
Re: [PATCH 04/11] drm/meson: Add G12A Support for VIU setup
On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote:
> Amlogic G12A SoC needs a different VIU setup code,
> handle it.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/meson/meson_viu.c | 72 ---
> 1 file changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_viu.c
> b/drivers/gpu/drm/meson/meson_viu.c
> index ac0f3687e09a..0169c98b01c9 100644
> --- a/drivers/gpu/drm/meson/meson_viu.c
> +++ b/drivers/gpu/drm/meson/meson_viu.c
> @@ -90,6 +90,34 @@ static int eotf_bypass_coeff[EOTF_COEFF_SIZE] = {
> EOTF_COEFF_RIGHTSHIFT /* right shift */
> };
>
> +void meson_viu_set_g12a_osd1_matrix(struct meson_drm *priv, int *m,
> +bool csc_on)
> +{
> + /* VPP WRAP OSD1 matrix */
> + writel(((m[0] & 0xfff) << 16) | (m[1] & 0xfff),
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_PRE_OFFSET0_1));
> + writel(m[2] & 0xfff,
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_PRE_OFFSET2));
> + writel(((m[3] & 0x1fff) << 16) | (m[4] & 0x1fff),
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_COEF00_01));
> + writel(((m[5] & 0x1fff) << 16) | (m[6] & 0x1fff),
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_COEF02_10));
> + writel(((m[7] & 0x1fff) << 16) | (m[8] & 0x1fff),
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_COEF11_12));
> + writel(((m[9] & 0x1fff) << 16) | (m[10] & 0x1fff),
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_COEF20_21));
> + writel((m[11] & 0x1fff) << 16,
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_COEF22));
> +
> + writel(((m[18] & 0xfff) << 16) | (m[19] & 0xfff),
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_OFFSET0_1));
Can you define some of the masks and shifts above ? possibly the same define
for all the registers I suppose ... maybe using FIELD_PREP ?
> + writel(m[20] & 0xfff,
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_OFFSET2));
> +
> + writel_bits_relaxed(BIT(0), csc_on ? BIT(0) : 0,
> + priv->io_base + _REG(VPP_WRAP_OSD1_MATRIX_EN_CTRL));
> +}
> +
> void meson_viu_set_osd_matrix(struct meson_drm *priv,
> enum viu_matrix_sel_e m_select,
> int *m, bool csc_on)
> @@ -336,14 +364,24 @@ void meson_viu_init(struct meson_drm *priv)
> if (meson_vpu_is_compatible(priv, "amlogic,meson-gxm-vpu") ||
> meson_vpu_is_compatible(priv, "amlogic,meson-gxl-vpu"))
> meson_viu_load_matrix(priv);
> + else if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu"))
> + meson_viu_set_g12a_osd1_matrix(priv, RGB709_to_YUV709l_coeff,
> +true);
>
> /* Initialize OSD1 fifo control register */
> reg = BIT(0) | /* Urgent DDR request priority */
> - (4 << 5) | /* hold_fifo_lines */
> - (3 << 10) | /* burst length 64 */
> - (32 << 12) | /* fifo_depth_val: 32*8=256 */
> - (2 << 22) | /* 4 words in 1 burst */
> - (2 << 24);
> + (4 << 5); /* hold_fifo_lines */
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu"))
> + reg |= (1 << 10) | /* burst length 32 */
> +(32 << 12) | /* fifo_depth_val: 32*8=256 */
> +(2 << 22) | /* 4 words in 1 burst */
> +(2 << 24) |
> +(1 << 31);
> + else
> + reg |= (3 << 10) | /* burst length 64 */
> +(32 << 12) | /* fifo_depth_val: 32*8=256 */
> +(2 << 22) | /* 4 words in 1 burst */
> +(2 << 24);
Could you use the BIT() macro and add some defines ?
> writel_relaxed(reg, priv->io_base + _REG(VIU_OSD1_FIFO_CTRL_STAT));
> writel_relaxed(reg, priv->io_base + _REG(VIU_OSD2_FIFO_CTRL_STAT));
>
> @@ -369,6 +407,30 @@ void meson_viu_init(struct meson_drm *priv)
> writel_relaxed(0x00FF00C0,
> priv->io_base + _REG(VD2_IF0_LUMA_FIFO_SIZE));
>
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
> + writel_relaxed(4 << 29 |
> + 1 << 27 |
> + 1 << 26 | /* blend_din0 input to blend0 */
> + 1 << 25 | /* blend1_dout to blend2 */
> + 1 << 24 | /* blend1_din3 input to blend1 */
> + 1 << 20 |
> + 0 << 16 |
> + 1,
> + priv->io_base + _REG(VIU_OSD_BLEND_CTRL));
> + writel_relaxed(3 << 8 |
> + 1 << 20,
> + priv->io_base + _REG(OSD1_BLEND_SRC_CTRL));
> + writel_relaxed(1 << 20,
> + priv->io_base + _REG(OSD2_BLEND_SRC_CTRL));
> + writel_relaxed(0,
Re: [PATCH 00/11] drm/meson: Add G12A Support
On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote: > The Amlogic G12A SoC offers very close Video Display > functionnalities with it's older GXBB, GXL & GXM predecessors. > > The main differences are : > - G12A Support now 3 "real" OSD planes with a new Blender module > - Instead of having a single Scaler for OSD1, G12A has two scaler > that can be applied to 2 out of the 3 OSD planes or on the outputs > of the blender module. > - The HDMI PHY now support RX-SENSE, Dynamic HDR and it's registers are > now memory mapped instead of using an internal bus. > - The VPU now support a DSI interface to connect a display, using > the Synopsys DSI controller and a custom PHY > > The complex Blender routing, HDMI RX-SENSE, Dynamic HDR and DSI support > are not handled in this patchset. > > This patchset implements on-par support with the currently support > GXBB, GXL and GXM SoCs. There is no support delta with this patchset. > > patch 10 & 11 implements the bindings found at [1]. > > [1] https://lkml.kernel.org/r/20190313141030.5958-1-narmstr...@baylibre.com > > Neil Armstrong (11): > drm/meson: Switch PLL to 5.94GHz base for 297Mhz pixel clock > drm/meson: Add registers for G12A SoC > drm/meson: Add G12A Support for VPP setup > drm/meson: Add G12A Support for VIU setup > drm/meson: Add G12A support for OSD1 Plane > drm/meson: Add G12A Support for the Overlay video plane > drm/meson: Add G12A support for plane handling in CRTC driver > drm/meson: Add G12A support for CVBS Encoer > drm/meson: Add G12A Video Clock setup > drm/meson: Add G12A compatible > drm/meson: Add G12A support for the DW-HDMI Glue > > drivers/gpu/drm/meson/meson_crtc.c | 269 +++- > drivers/gpu/drm/meson/meson_drv.c | 1 + > drivers/gpu/drm/meson/meson_drv.h | 4 + > drivers/gpu/drm/meson/meson_dw_hdmi.c | 163 +++--- > drivers/gpu/drm/meson/meson_dw_hdmi.h | 32 ++- > drivers/gpu/drm/meson/meson_overlay.c | 10 +- > drivers/gpu/drm/meson/meson_plane.c | 15 +- > drivers/gpu/drm/meson/meson_registers.h | 247 ++ > drivers/gpu/drm/meson/meson_vclk.c | 123 +-- > drivers/gpu/drm/meson/meson_venc.c | 11 +- > drivers/gpu/drm/meson/meson_venc_cvbs.c | 25 ++- > drivers/gpu/drm/meson/meson_viu.c | 72 ++++++- > drivers/gpu/drm/meson/meson_vpp.c | 51 +++-- > 13 files changed, 880 insertions(+), 143 deletions(-) > on the u200 and sei510 Tested-by: Jerome Brunet I can't pretend to have a deep understanding of this subsystem, but as far as I can tell, there is nothing crazy in there. With the comments around the use of constants and defines fixed, you can add Reviewed-by: Jerome Brunet As a possible future enhancement, it would be nice if you could trim the use of *_is_compatible() functions. I think it would make the code easier to follow and review. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/11] drm/meson: Add G12A support for CVBS Encoer
On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote:
> The Meson G12A SoCs uses the exact same CVBS encoder except a simple
> CVBS DAC register offset and settings delta.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/meson/meson_venc.c | 11 +--
> drivers/gpu/drm/meson/meson_venc_cvbs.c | 25 ++---
> 2 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_venc.c
> b/drivers/gpu/drm/meson/meson_venc.c
> index 66d73a932d19..6faca7313339 100644
> --- a/drivers/gpu/drm/meson/meson_venc.c
> +++ b/drivers/gpu/drm/meson/meson_venc.c
> @@ -73,7 +73,9 @@
> /* HHI Registers */
> #define HHI_GCLK_MPEG2 0x148 /* 0x52 offset in data sheet */
> #define HHI_VDAC_CNTL0 0x2F4 /* 0xbd offset in data sheet */
> +#define HHI_VDAC_CNTL0_G12A 0x2EC /* 0xbd offset in data sheet */
> #define HHI_VDAC_CNTL1 0x2F8 /* 0xbe offset in data sheet */
> +#define HHI_VDAC_CNTL1_G12A 0x2F0 /* 0xbe offset in data sheet */
> #define HHI_HDMI_PHY_CNTL0 0x3a0 /* 0xe8 offset in data sheet */
>
> struct meson_cvbs_enci_mode meson_cvbs_enci_pal = {
> @@ -1675,8 +1677,13 @@ void meson_venc_disable_vsync(struct meson_drm *priv)
> void meson_venc_init(struct meson_drm *priv)
> {
> /* Disable CVBS VDAC */
> - regmap_write(priv->hhi, HHI_VDAC_CNTL0, 0);
> - regmap_write(priv->hhi, HHI_VDAC_CNTL1, 8);
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
> + regmap_write(priv->hhi, HHI_VDAC_CNTL0_G12A, 0);
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1_G12A, 8);
> + } else {
> + regmap_write(priv->hhi, HHI_VDAC_CNTL0, 0);
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1, 8);
> + }
>
> /* Power Down Dacs */
> writel_relaxed(0xff, priv->io_base + _REG(VENC_VDAC_SETTING));
> diff --git a/drivers/gpu/drm/meson/meson_venc_cvbs.c
> b/drivers/gpu/drm/meson/meson_venc_cvbs.c
> index d622d817b6df..2c5341c881c4 100644
> --- a/drivers/gpu/drm/meson/meson_venc_cvbs.c
> +++ b/drivers/gpu/drm/meson/meson_venc_cvbs.c
> @@ -37,7 +37,9 @@
>
> /* HHI VDAC Registers */
> #define HHI_VDAC_CNTL0 0x2F4 /* 0xbd offset in data sheet */
> +#define HHI_VDAC_CNTL0_G12A 0x2EC /* 0xbd offset in data sheet */
> #define HHI_VDAC_CNTL1 0x2F8 /* 0xbe offset in data sheet */
> +#define HHI_VDAC_CNTL1_G12A 0x2F0 /* 0xbe offset in data sheet */
>
> struct meson_venc_cvbs {
> struct drm_encoder encoder;
> @@ -166,8 +168,13 @@ static void meson_venc_cvbs_encoder_disable(struct
> drm_encoder *encoder)
> struct meson_drm *priv = meson_venc_cvbs->priv;
>
> /* Disable CVBS VDAC */
> - regmap_write(priv->hhi, HHI_VDAC_CNTL0, 0);
> - regmap_write(priv->hhi, HHI_VDAC_CNTL1, 8);
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
> + regmap_write(priv->hhi, HHI_VDAC_CNTL0_G12A, 0);
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1_G12A, 0);
> + } else {
> + regmap_write(priv->hhi, HHI_VDAC_CNTL0, 0);
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1, 8);
I imagine 8 stands for BIT(3) ? Could you add a define to explain (quickly)
what it is ?
> + }
> }
>
> static void meson_venc_cvbs_encoder_enable(struct drm_encoder *encoder)
> @@ -179,13 +186,17 @@ static void meson_venc_cvbs_encoder_enable(struct
> drm_encoder *encoder)
> /* VDAC0 source is not from ATV */
> writel_bits_relaxed(BIT(5), 0, priv->io_base + _REG(VENC_VDAC_DACSEL0));
>
> - if (meson_vpu_is_compatible(priv, "amlogic,meson-gxbb-vpu"))
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-gxbb-vpu")) {
> regmap_write(priv->hhi, HHI_VDAC_CNTL0, 1);
> - else if (meson_vpu_is_compatible(priv, "amlogic,meson-gxm-vpu") ||
> - meson_vpu_is_compatible(priv, "amlogic,meson-gxl-vpu"))
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1, 0);
> + } else if (meson_vpu_is_compatible(priv, "amlogic,meson-gxm-vpu") ||
> + meson_vpu_is_compatible(priv, "amlogic,meson-gxl-vpu")) {
> regmap_write(priv->hhi, HHI_VDAC_CNTL0, 0xf0001);
> -
> - regmap_write(priv->hhi, HHI_VDAC_CNTL1, 0);
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1, 0);
> + } else if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
> + regmap_write(priv->hhi, HHI_VDAC_CNTL0_G12A, 0x906001);
> + regmap_write(priv->hhi, HHI_VDAC_CNTL1_G12A, 0);
> + }
Maybe the values above are just magics taken from the vendor kernel, but if
you can, it would be nice to break them down to help us understand what is
controlled in these CTNL registers.
> }
>
> static void meson_venc_cvbs_encoder_mode_set(struct drm_encoder *encoder,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
Re: [PATCH 06/11] drm/meson: Add G12A Support for the Overlay video plane
On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote:
> Amlogic G12A SoC supports the same set of Video Planes, but now
> are handled by the new OSD plane blender module.
>
> This patch uses the same VD1 plane for G12A, using the exact same scaler
> and VD11 setup registers, except using the new blender register to
> disable the plane.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/meson/meson_overlay.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_overlay.c
> b/drivers/gpu/drm/meson/meson_overlay.c
> index b54a22e483b9..bdbf925ff3e8 100644
> --- a/drivers/gpu/drm/meson/meson_overlay.c
> +++ b/drivers/gpu/drm/meson/meson_overlay.c
> @@ -516,8 +516,14 @@ static void meson_overlay_atomic_disable(struct
> drm_plane *plane,
> priv->viu.vd1_enabled = false;
>
> /* Disable VD1 */
> - writel_bits_relaxed(VPP_VD1_POSTBLEND | VPP_VD1_PREBLEND, 0,
> - priv->io_base + _REG(VPP_MISC));
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
> + writel_relaxed(0, priv->io_base + _REG(VD1_BLEND_SRC_CTRL));
> + writel_relaxed(0, priv->io_base + _REG(VD2_BLEND_SRC_CTRL));
> + writel_relaxed(0, priv->io_base + _REG(VD1_IF0_GEN_REG +
> 0x17b0));
> + writel_relaxed(0, priv->io_base + _REG(VD2_IF0_GEN_REG +
> 0x17b0));
Is it possible to add a comment explaining this 0x17b0 value ?
> + } else
> + writel_bits_relaxed(VPP_VD1_POSTBLEND | VPP_VD1_PREBLEND, 0,
> + priv->io_base + _REG(VPP_MISC));
>
> }
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/11] drm/meson: Add G12A support for CVBS Encoer
On Tue, 2019-04-09 at 10:43 +0200, Jerome Brunet wrote: > On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote: > > The Meson G12A SoCs uses the exact same CVBS encoder except a simple > > CVBS DAC register offset and settings delta. > > > > Signed-off-by: Neil Armstrong > > --- > > drivers/gpu/drm/meson/meson_venc.c | 11 +-- > > drivers/gpu/drm/meson/meson_venc_cvbs.c | 25 ++--- > > 2 files changed, 27 insertions(+), 9 deletions(-) ... and there is a typo in the patch title. s/Encoer/Encoder ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/11] drm/meson: Add G12A Video Clock setup
On Mon, 2019-03-25 at 15:18 +0100, Neil Armstrong wrote:
> While switching to the Common Clock Framework is still Work In Progress,
> this patch adds the corresponding G12A HDMI PLL setup to be on-par
> with the other SoCs support.
>
> The G12A has only a single tweak about the high frequency setup,
> where the HDMI PLL needs a specific setup to handle correctly the
> 5.94GHz DCO frequency.
>
> Apart that, it handle correctly all the other HDMI frequencies
> and can achieve even better DMT clock frequency precision with
> the larger fractional dividier width.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/meson/meson_vclk.c | 119 ++---
> 1 file changed, 108 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_vclk.c
> b/drivers/gpu/drm/meson/meson_vclk.c
> index c15a5a5df633..b39034745444 100644
> --- a/drivers/gpu/drm/meson/meson_vclk.c
> +++ b/drivers/gpu/drm/meson/meson_vclk.c
> @@ -113,9 +113,12 @@
> #define HHI_HDMI_PLL_CNTL4 0x32C /* 0xcb offset in data sheet */
> #define HHI_HDMI_PLL_CNTL5 0x330 /* 0xcc offset in data sheet */
> #define HHI_HDMI_PLL_CNTL6 0x334 /* 0xcd offset in data sheet */
> +#define HHI_HDMI_PLL_CNTL7 0x338 /* 0xce offset in data sheet */
>
> #define HDMI_PLL_RESET BIT(28)
> +#define HDMI_PLL_RESET_G12A BIT(29)
> #define HDMI_PLL_LOCKBIT(31)
> +#define HDMI_PLL_LOCK_G12A (3 << 30)
GENMASK(31, 30) ?
>
> #define FREQ_1000_1001(_freq)DIV_ROUND_CLOSEST(_freq * 1000, 1001)
>
> @@ -257,6 +260,10 @@ static void meson_venci_cvbs_clock_config(struct
> meson_drm *priv)
> regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL5, 0x71486980);
> regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL6, 0x0e55);
> regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL, 0x4800023d);
> +
> + /* Poll for lock bit */
> + regmap_read_poll_timeout(priv->hhi, HHI_HDMI_PLL_CNTL, val,
> + (val & HDMI_PLL_LOCK), 10, 0);
> } else if (meson_vpu_is_compatible(priv, "amlogic,meson-gxm-vpu") ||
> meson_vpu_is_compatible(priv, "amlogic,meson-gxl-vpu")) {
> regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL, 0x427b);
> @@ -271,11 +278,26 @@ static void meson_venci_cvbs_clock_config(struct
> meson_drm *priv)
> HDMI_PLL_RESET, HDMI_PLL_RESET);
> regmap_update_bits(priv->hhi, HHI_HDMI_PLL_CNTL,
> HDMI_PLL_RESET, 0);
> - }
>
> - /* Poll for lock bit */
> - regmap_read_poll_timeout(priv->hhi, HHI_HDMI_PLL_CNTL, val,
> - (val & HDMI_PLL_LOCK), 10, 0);
> + /* Poll for lock bit */
> + regmap_read_poll_timeout(priv->hhi, HHI_HDMI_PLL_CNTL, val,
> + (val & HDMI_PLL_LOCK), 10, 0);
> + } else if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu")) {
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL, 0x1a0504f7);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL2, 0x0001);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL3, 0x);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL4, 0x6a28dc00);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL5, 0x65771290);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL6, 0x39272000);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL7, 0x5654);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL, 0x3a0504f7);
> + regmap_write(priv->hhi, HHI_HDMI_PLL_CNTL, 0x1a0504f7);
> +
> + /* Poll for lock bit */
> + regmap_read_poll_timeout(priv->hhi, HHI_HDMI_PLL_CNTL, val,
> + ((val & HDMI_PLL_LOCK_G12A) == HDMI_PLL_LOCK_G12A),
> + 10, 0);
> + }
>
> /* Disable VCLK2 */
> regmap_update_bits(priv->hhi, HHI_VIID_CLK_CNTL, VCLK2_EN, 0);
> @@ -288,8 +310,13 @@ static void meson_venci_cvbs_clock_config(struct
> meson_drm *priv)
> VCLK2_DIV_MASK, (55 - 1));
>
> /* select vid_pll for vclk2 */
> - regmap_update_bits(priv->hhi, HHI_VIID_CLK_CNTL,
> - VCLK2_SEL_MASK, (4 << VCLK2_SEL_SHIFT));
> + if (meson_vpu_is_compatible(priv, "amlogic,meson-g12a-vpu"))
> + regmap_update_bits(priv->hhi, HHI_VIID_CLK_CNTL,
> + VCLK2_SEL_MASK, (0 << VCLK2_SEL_SHIFT));
> + else
> + regmap_update_bits(priv->hhi, HHI_VIID_CLK_CNTL,
> + VCLK2_SEL_MASK, (4 << VCLK2_SEL_SHIFT));
> +
> /* enable vclk2 gate */
> regmap_update_bits(priv->hhi, HHI_VIID_CLK_CNTL, VCLK2_EN, VCLK2_EN);
>
> @@ -476,32 +503,80 @@ void meson_hdmi_pll_set_params(struct meson_drm *priv,
> unsigned int m,
> /* Poll for lock bit */
>
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
On Wed, 2018-08-01 at 20:51 +0200, Maxime Jourdan wrote:
> This removes the meson_canvas files within the meson/drm layer
> and makes use of the new canvas module that is referenced in the dts.
>
> Canvases can be used by different IPs and modules, and it is as such
> preferable to rely on a module that can safely dispatch canvases on
> demand.
>
> Signed-off-by: Maxime Jourdan
> ---
> .../bindings/display/amlogic,meson-vpu.txt| 9 +--
> arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 7 +-
> drivers/gpu/drm/meson/Kconfig | 1 +
> drivers/gpu/drm/meson/Makefile| 2 +-
> drivers/gpu/drm/meson/meson_canvas.c | 70 ---
> drivers/gpu/drm/meson/meson_canvas.h | 42 ---
> drivers/gpu/drm/meson/meson_crtc.c| 5 +-
> drivers/gpu/drm/meson/meson_drv.c | 35 ++
> drivers/gpu/drm/meson/meson_drv.h | 5 +-
> drivers/gpu/drm/meson/meson_plane.c | 3 +-
> drivers/gpu/drm/meson/meson_viu.c | 1 -
> 11 files changed, 39 insertions(+), 141 deletions(-)
> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c
> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h
>
> diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> index 057b81335775..60b6e1398636 100644
> --- a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> +++ b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> @@ -60,9 +60,9 @@ Required properties:
> - reg: base address and size of he following memory-mapped regions :
> - vpu
> - hhi
> - - dmc
> - reg-names: should contain the names of the previous memory regions
> - interrupts: should contain the VENC Vsync interrupt number
> +- amlogic,canvas: should point to a meson canvas provider node
>
> Optional properties:
> - power-domains: Optional phandle to associated power domain as described in
> @@ -98,13 +98,14 @@ tv-connector {
> vpu: vpu@d010 {
> compatible = "amlogic,meson-gxbb-vpu";
> reg = <0x0 0xd010 0x0 0x10>,
> - <0x0 0xc883c000 0x0 0x1000>,
> - <0x0 0xc8838000 0x0 0x1000>;
> - reg-names = "vpu", "hhi", "dmc";
> + <0x0 0xc883c000 0x0 0x1000>;
> + reg-names = "vpu", "hhi";
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
>
> + amlogic,canvas = <>;
> +
> /* CVBS VDAC output port */
> port@0 {
> reg = <0>;
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index d104b9e111fb..7c4d971ecd80 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -503,13 +503,14 @@
> vpu: vpu@d010 {
> compatible = "amlogic,meson-gx-vpu";
> reg = <0x0 0xd010 0x0 0x10>,
> - <0x0 0xc883c000 0x0 0x1000>,
> - <0x0 0xc8838000 0x0 0x1000>;
> - reg-names = "vpu", "hhi", "dmc";
> + <0x0 0xc883c000 0x0 0x1000>;
> + reg-names = "vpu", "hhi";
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
>
> + amlogic,canvas = <>;
> +
> /* CVBS VDAC output port */
> cvbs_vdac_port: port@0 {
> reg = <0>;
> diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig
> index 3ce51d8dfe1c..c28b69f48555 100644
> --- a/drivers/gpu/drm/meson/Kconfig
> +++ b/drivers/gpu/drm/meson/Kconfig
> @@ -7,6 +7,7 @@ config DRM_MESON
> select DRM_GEM_CMA_HELPER
> select VIDEOMODE_HELPERS
> select REGMAP_MMIO
> + select MESON_CANVAS
>
> config DRM_MESON_DW_HDMI
> tristate "HDMI Synopsys Controller support for Amlogic Meson Display"
> diff --git a/drivers/gpu/drm/meson/Makefile b/drivers/gpu/drm/meson/Makefile
> index c5c4cc362f02..bd67429185ff 100644
> --- a/drivers/gpu/drm/meson/Makefile
> +++ b/drivers/gpu/drm/meson/Makefile
> @@ -1,5 +1,5 @@
> meson-drm-y := meson_drv.o meson_plane.o meson_crtc.o meson_venc_cvbs.o
> -meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o
> meson_canvas.o
> +meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o
>
> obj-$(CONFIG_DRM_MESON) += meson-drm.o
> obj-$(CONFIG_DRM_MESON_DW_HDMI) += meson_dw_hdmi.o
> diff --git a/drivers/gpu/drm/meson/meson_canvas.c
> b/drivers/gpu/drm/meson/meson_canvas.c
> deleted file mode 100644
> index 08f6073d967e..
> --- a/drivers/gpu/drm/meson/meson_canvas.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -/*
> - * Copyright (C) 2016 BayLibre, SAS
> - * Author: Neil Armstrong
> - * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> -
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
On Thu, 2018-08-02 at 14:34 +0200, Maxime Jourdan wrote: > Hi Jerome, > > 2018-08-02 10:39 GMT+02:00 Jerome Brunet : > > I looks like the consumer of your 'canvas' devices must know how the canvas > > device is organized internally. Maybe something better can be done ? > > > > Your canvas driver could provide a consumer API, for example: > > meson_canvas_get(): to translate for struct device_node to whatever abstract > > pointer you would need. > > meson_canvas_alloc(), setup(), etc ... > > > > ... This is just adding a bit of indirection but it would help hide the > > plumbing > > of your canvas driver from the consumers (and repeat this code in each). > > This > > might be usefull if you ever to make this canvas driver evolve. > > Overall the inner workings are hidden as there is an ops struct > instead of public functions. What I don't like is precisely that you need to expose this 'struct ops' to the consumer. I would prefer an API for this (it can be a 1:1 mapping). The canvas should remain some abstract object you get from DT. IMO, this is the same as a reset, a syscon or whatever other phandle you get from DT. The consumer should not have to 'care' how it works (how data are organized), it should just use it. Whatever you need to do to deal with your canvas phandle should (IMO) reside in your soc/amlogic/meson-canvas.c, and not be spread in the consumers. > > I agree that the "fetch the node" boilerplate code could be put behind > a helper, but at the same time this code helps remind the developer > that there needs to be a canvas node in the dts, and that it has to be > linked in your own device node. This is why we have a DT documentation. And, as far as I understand amlogic's display and decoder stuff, you won't get very far w/o a canvas, so 'the developer' will be reminded fairly quickly ;) > > I would like to keep it that way if that is okay with you. It's just my opinion, I'm not the one merging it ... :P ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RESEND] drm/meson: Make DMT timings parameters and pixel clock generic
On Mon, 2018-07-16 at 09:40 +0200, Neil Armstrong wrote:
> Remove the modes timings tables for DMT modes and calculate the HW
> paremeters from the modes timings.
>
> Switch the DMT modes pixel clock calculation out of the static frequency
> list to a generic calculation from a range of possible PLL dividers.
>
> This patch is an intermediate step towards usage of the Common Clock
> Framwework for PLL setup, by reworking the code to have common
> sel_pll() function called by the CEA (HDMI) freq setup and the generic
> DMT frequencies setup, we should be able to simply call clk_set_rate()
> on the PLL clock handle in a near future.
>
> The CEA (HDMI) and CVBS modes needs very specific clock paths that CCF will
> never be able to determine by itself, so there is still some work to do for
> a full handoff to CCF handling the clocks.
Patch seems to be a good step forward making the display compatible with CCF
indeed. While full automatic handling through CCF might not possible, it would
be good if, someday, we could handle the SoC quirks in CCF, removing the need
check is the SoC is gxbb, gxl or gxm while setting the clocks.
If the display driver needs a detailed control over the clock setup, maybe we
could solve the problem by exporting the intermediate clock elements in CCF
(such as muxes, ODs, etc...) and let the display driver claim them all ?
Anyway, the situation is improving so:
Acked-by: Jerome Brunet
>
> This setup permits setting non-CEA modes like :
> - 1600x900-60Hz
> - 1280x1024-75Hz
> - 1280x1024-60Hz
> - 1440x900-60Hz
> - 1366x768-60Hz
> - 1280x800-60Hz
> - 1152x864-75Hz
> - 1024x768-75Hz
> - 1024x768-70Hz
> - 1024x768-60Hz
> - 832x624-75Hz
> - 800x600-75Hz
> - 800x600-72Hz
> - 800x600-60Hz
> - 640x480-75Hz
> - 640x480-73Hz
> - 640x480-67Hz
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 22 +-
> drivers/gpu/drm/meson/meson_vclk.c| 672
> +++---
> drivers/gpu/drm/meson/meson_vclk.h| 4 +
> drivers/gpu/drm/meson/meson_venc.c| 377 +++
> drivers/gpu/drm/meson/meson_venc.h| 3 +-
> 5 files changed, 358 insertions(+), 720 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index c9ad456..df7247c 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -329,6 +329,12 @@ static void dw_hdmi_set_vclk(struct meson_dw_hdmi
> *dw_hdmi,
>
> vclk_freq = mode->clock;
>
> + if (!vic) {
> + meson_vclk_setup(priv, MESON_VCLK_TARGET_DMT, vclk_freq,
> + vclk_freq, vclk_freq, false);
> + return;
> + }
> +
> if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> vclk_freq *= 2;
>
> @@ -542,10 +548,12 @@ static enum drm_mode_status
> dw_hdmi_mode_valid(struct drm_connector *connector,
> const struct drm_display_mode *mode)
> {
> + struct meson_drm *priv = connector->dev->dev_private;
> unsigned int vclk_freq;
> unsigned int venc_freq;
> unsigned int hdmi_freq;
> int vic = drm_match_cea_mode(mode);
> + enum drm_mode_status status;
>
> DRM_DEBUG_DRIVER("Modeline %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x
> 0x%x\n",
> mode->base.id, mode->name, mode->vrefresh, mode->clock,
> @@ -556,8 +564,11 @@ dw_hdmi_mode_valid(struct drm_connector *connector,
>
> /* Check against non-VIC supported modes */
> if (!vic) {
> - if (!meson_venc_hdmi_supported_mode(mode))
> - return MODE_BAD;
> + status = meson_venc_hdmi_supported_mode(mode);
> + if (status != MODE_OK)
> + return status;
> +
> + return meson_vclk_dmt_supported_freq(priv, mode->clock);
> /* Check against supported VIC modes */
> } else if (!meson_venc_hdmi_supported_vic(vic))
> return MODE_BAD;
> @@ -583,16 +594,11 @@ dw_hdmi_mode_valid(struct drm_connector *connector,
> dev_dbg(connector->dev->dev, "%s: vclk:%d venc=%d hdmi=%d\n", __func__,
> vclk_freq, venc_freq, hdmi_freq);
>
> - /* Finally filter by configurable vclk frequencies */
> + /* Finally filter by configurable vclk frequencies for VIC modes */
> switch (vclk_freq) {
> - case 25175:
> - case 4:
> case 54000:
> - case 65000:
> case 74250:
> - case 108000:
> case 148500:
> - case 162000:
> case 297000:
> case 594000:
>
Re: [PATCH] drm/meson: Add support for DMT modes on HDMI
On Tue, 2018-03-13 at 11:07 +0100, Neil Armstrong wrote: > This patch adds support for DMT display modes over HDMI. > The modes timings configurations are from the Amlogic Vendor linux tree > and tested over multiples monitors. > Previously only a selected number of CEA modes were supported. > > Only these following modes are supported with these changes: > - 640x480@60Hz > - 800x600@60Hz > - 1024x768@60Hz > - 1152x864@75Hz > - 1280x1024@60Hz > - 1600x1200@60Hz > - 1920x1080@60Hz > > The associated code to handle the clock rates is also added. > > Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> Looks good to me Acked-by: Jerome Brunet <jbru...@baylibre.com> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RESEND PATCH 0/4] drm/meson: power domain init related fixes
On Wed, 2017-12-06 at 12:54 +0100, Neil Armstrong wrote: > On the Amlogic Gx SoCs (GXBB, GXL & GXM), the VPU power domain is initialized > by the vendor U-Boot code, but running mainline U-boot has been possible > on these SoCs. But lacking such init made the system lock at kernel boot. > > A PM Power Domain driver has been pushed at [1] to solve the main issue. > The following patches : > - updates the DT bindings accordingly > - adds support for missing regulators and registers init > > Neil Armstrong (4): > dt-bindings: display: amlogic,meson-vpu: Add optional power domain > property > dt-bindings: display: amlogic,meson-dw-hdmi: Add optional HDMI 5V > regulator > drm/meson: dw_hdmi: Add support for an optional external 5V regulator > drm/meson: Add missing VPU init > > .../devicetree/bindings/display/amlogic,meson-dw-hdmi.txt | 4 > .../devicetree/bindings/display/amlogic,meson-vpu.txt | 4 > drivers/gpu/drm/meson/meson_drv.c | 9 + > drivers/gpu/drm/meson/meson_dw_hdmi.c | 13 > + > drivers/gpu/drm/meson/meson_registers.h | 4 > 5 files changed, 34 insertions(+) > No dependencies on the bootloader anymore, this is great ! Thanks Series tested on libretech-cc s905x Tested-by: Jerome Brunet <jbru...@baylibre.com> Reviewed-by: Jerome Brunet <jbru...@baylibre.com> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
