| Message ID | 20180902072643.4917-1-jernej.skrabec@siol.net |
|---|---|
| Headers | show |
| Series | Allwinner H6 DE3 and HDMI support | expand |
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > This series adds support for Display Engine 3.0 and HDMI 2.0a, which > can be found on H6 SoC. > > Display Engine 3.0 in comparison to 2.0 mostly adds features needed for > displaying and processing 10-bit and AFBC formats, which are not yet > supported by this series. > > This series is based on linux-next at next-20180828, which has working > R40 display pipeline support. I'll rebase series on later linux-next, if > needed, once R40 display pipeline support is reintroduced. > > I suggest all patches go through allwinner tree, except DRM patches, > which should go through drm-misc tree. > > Last detail, PineH64 model A schematic has DDC_EN signal, which enables > DDC voltage level shifter. TL Lim, PINE64 founder, said that this > signal is not actually present on PineH64 model A board. It is, however > present on PineH64 model B engineering samples, but it will be removed > in production version. Because of that, I didn't include any code for > it. > > Please take a look. > > Best regards, > Jernej > > Icenowy Zheng (7): > dt-bindings: sunxi-sram: add binding for Allwinner H6 SRAM C > arm64: allwinner: h6: add system controller device tree node Prefix should be "arm64: dts: allwinner: h6: ". > dt-bindings: bus: add H6 DE3 bus binding > dt-bindings: display: sunxi: add DT binding for Allwinner H6 DW HDMI > drm: sun4i: add quirks for TCON TOP > dt-bindings: display: sun4i-drm: document H6 TCON TOP > drm: sun4i: add support for H6 TCON TOP > > Jernej Skrabec (20): > clk: sunxi-ng: Adjust MP clock parent rate when allowed > clk: sunxi-ng: Use u64 for calculation of NM rate > clk: sunxi-ng: h6: Set video PLLs limits > dt-bindings: clock: sun8i-de2: Add H6 DE3 clock description > clk: sunxi-ng: Add support for H6 DE3 clocks > dt-bindings: display: sun4i-drm: Add H6 display engine compatibles > drm/sun4i: Add compatible for H6 display engine > drm/sun4i: Rework DE2 register defines > drm/sun4i: Add basic support for DE3 > drm/sun4i: Add support for H6 DE3 mixer 0 > drm/bridge/synopsys: dw-hdmi: Enable workaround for v2.12a > drm/sun4i: Not all DW HDMI controllers has scrambled addresses > drm/sun4i: dw-hdmi: Make mode_valid function configurable > drm/sun4i: dw-hdmi: Add quirk for setting TMDS clock > drm/sun4i: Add support for H6 DW HDMI controller > drm/sun4i: Add support for Synopsys HDMI PHY > drm/sun4i: Add support for H6 HDMI PHY > drm/sun4i: Initialize registers in tcon-top driver > arm64: dts: sun50i: h6: Add HDMI pipeline > arm64: dts: sun50i: h6: Enable HDMI output on Pine H64 board Same here. ChenYu > > .../bindings/bus/sun50i-de2-bus.txt | 9 +- > .../devicetree/bindings/clock/sun8i-de2.txt | 5 +- > .../bindings/display/sunxi/sun4i-drm.txt | 30 ++- > .../devicetree/bindings/sram/sunxi-sram.txt | 4 + > .../boot/dts/allwinner/sun50i-h6-pine-h64.dts | 25 ++ > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 224 ++++++++++++++++++ > drivers/clk/sunxi-ng/ccu-sun50i-h6.c | 4 + > drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 65 +++++ > drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 1 + > drivers/clk/sunxi-ng/ccu_mp.c | 64 ++++- > drivers/clk/sunxi-ng/ccu_nm.c | 18 +- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 + > drivers/gpu/drm/sun4i/sun4i_drv.c | 1 + > drivers/gpu/drm/sun4i/sun8i_csc.c | 96 +++++++- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 45 +++- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 14 +- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 178 +++++++++++++- > drivers/gpu/drm/sun4i/sun8i_mixer.c | 44 +++- > drivers/gpu/drm/sun4i/sun8i_mixer.h | 61 +++-- > drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 58 ++++- > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 47 ++-- > drivers/gpu/drm/sun4i/sun8i_ui_layer.h | 37 +-- > drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 45 ++-- > drivers/gpu/drm/sun4i/sun8i_ui_scaler.h | 28 +-- > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 55 +++-- > drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 25 +- > drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 85 +++++-- > drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 68 ++++-- > include/dt-bindings/clock/sun8i-de2.h | 3 + > include/dt-bindings/reset/sun8i-de2.h | 1 + > 30 files changed, 1127 insertions(+), 214 deletions(-) > > -- > 2.18.0 >
hi, On Sun, Sep 02, 2018 at 09:26:26AM +0200, Jernej Skrabec wrote: > H6 is first Allwinner SoC which supports 10 bit colors, HDR and AFBC. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun4i_drv.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c > index dd19d674055c..e5731d092e1a 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c > @@ -410,6 +410,7 @@ static int sun4i_drv_remove(struct platform_device *pdev) > > static const struct of_device_id sun4i_drv_of_table[] = { > { .compatible = "allwinner,sun4i-a10-display-engine" }, > + { .compatible = "allwinner,sun50i-h6-display-engine" }, > { .compatible = "allwinner,sun5i-a10s-display-engine" }, > { .compatible = "allwinner,sun5i-a13-display-engine" }, > { .compatible = "allwinner,sun6i-a31-display-engine" }, While it makes sens from an alphabetical point of view, we usually follow the number order here (so after sun9i). Maxime
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > From: Icenowy Zheng <icenowy@aosc.io> > > As we have already binding for the H6 system controller, add its node > to the device tree. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > [fixed compatible string] > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
于 2018年9月4日 GMT+08:00 下午4:40:56, Chen-Yu Tsai <wens@csie.org> 写到: >On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> >wrote: >> >> From: Icenowy Zheng <icenowy@aosc.io> >> >> As we have already binding for the H6 system controller, add its node >> to the device tree. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> [fixed compatible string] >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > >Reviewed-by: Chen-Yu Tsai <wens@csie.org> By the way can these patches be applied first? (patches related to syscon). They're also needed for EMAC.
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Support for mixer0, mixer1, writeback and rotation units is added. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 65 ++++++++++++++++++++++++++++ > drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 1 + > 2 files changed, 66 insertions(+) > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > index bae5ee67a797..4535c1c27d27 100644 > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > @@ -31,6 +31,8 @@ static SUNXI_CCU_GATE(bus_mixer1_clk, "bus-mixer1", "bus-de", > 0x04, BIT(1), 0); > static SUNXI_CCU_GATE(bus_wb_clk, "bus-wb", "bus-de", > 0x04, BIT(2), 0); > +static SUNXI_CCU_GATE(bus_rot_clk, "bus-rot", "bus-de", > + 0x04, BIT(3), 0); > > static SUNXI_CCU_GATE(mixer0_clk, "mixer0", "mixer0-div", > 0x00, BIT(0), CLK_SET_RATE_PARENT); > @@ -38,6 +40,8 @@ static SUNXI_CCU_GATE(mixer1_clk, "mixer1", "mixer1-div", > 0x00, BIT(1), CLK_SET_RATE_PARENT); > static SUNXI_CCU_GATE(wb_clk, "wb", "wb-div", > 0x00, BIT(2), CLK_SET_RATE_PARENT); > +static SUNXI_CCU_GATE(rot_clk, "rot", "rot-div", > + 0x00, BIT(3), CLK_SET_RATE_PARENT); > > static SUNXI_CCU_M(mixer0_div_clk, "mixer0-div", "de", 0x0c, 0, 4, > CLK_SET_RATE_PARENT); > @@ -45,6 +49,8 @@ static SUNXI_CCU_M(mixer1_div_clk, "mixer1-div", "de", 0x0c, 4, 4, > CLK_SET_RATE_PARENT); > static SUNXI_CCU_M(wb_div_clk, "wb-div", "de", 0x0c, 8, 4, > CLK_SET_RATE_PARENT); > +static SUNXI_CCU_M(rot_div_clk, "rot-div", "de", 0x0c, 0x0c, 4, > + CLK_SET_RATE_PARENT); > > static SUNXI_CCU_M(mixer0_div_a83_clk, "mixer0-div", "pll-de", 0x0c, 0, 4, > CLK_SET_RATE_PARENT); > @@ -53,6 +59,24 @@ static SUNXI_CCU_M(mixer1_div_a83_clk, "mixer1-div", "pll-de", 0x0c, 4, 4, > static SUNXI_CCU_M(wb_div_a83_clk, "wb-div", "pll-de", 0x0c, 8, 4, > CLK_SET_RATE_PARENT); > > +static struct ccu_common *sun50i_h6_de3_clks[] = { > + &mixer0_clk.common, > + &mixer1_clk.common, > + &wb_clk.common, > + > + &bus_mixer0_clk.common, > + &bus_mixer1_clk.common, > + &bus_wb_clk.common, > + > + &mixer0_div_clk.common, > + &mixer1_div_clk.common, > + &wb_div_clk.common, > + > + &bus_rot_clk.common, > + &rot_clk.common, > + &rot_div_clk.common, > +}; > + > static struct ccu_common *sun8i_a83t_de2_clks[] = { > &mixer0_clk.common, > &mixer1_clk.common, > @@ -92,6 +116,26 @@ static struct ccu_common *sun8i_v3s_de2_clks[] = { > &wb_div_clk.common, > }; > > +static struct clk_hw_onecell_data sun50i_h6_de3_hw_clks = { > + .hws = { > + [CLK_MIXER0] = &mixer0_clk.common.hw, > + [CLK_MIXER1] = &mixer1_clk.common.hw, > + [CLK_WB] = &wb_clk.common.hw, > + [CLK_ROT] = &rot_clk.common.hw, > + > + [CLK_BUS_MIXER0] = &bus_mixer0_clk.common.hw, > + [CLK_BUS_MIXER1] = &bus_mixer1_clk.common.hw, > + [CLK_BUS_WB] = &bus_wb_clk.common.hw, > + [CLK_BUS_ROT] = &bus_rot_clk.common.hw, > + > + [CLK_MIXER0_DIV] = &mixer0_div_clk.common.hw, > + [CLK_MIXER1_DIV] = &mixer1_div_clk.common.hw, > + [CLK_WB_DIV] = &wb_div_clk.common.hw, > + [CLK_ROT_DIV] = &rot_div_clk.common.hw, > + }, > + .num = 12, It's best not to openly code these. It is error prone, like having an index beyond .num, which then never gets registered. Instead, please update CLK_NUMBERS and use that instead. sunxi_ccu_probe() can handle holes in .hws. On the other hand, it can't handle holes in the ccu_reset_map. Hope we never have to deal with such an instance. ChenYu > +}; > + > static struct clk_hw_onecell_data sun8i_a83t_de2_hw_clks = { > .hws = { > [CLK_MIXER0] = &mixer0_clk.common.hw, > @@ -156,6 +200,13 @@ static struct ccu_reset_map sun50i_a64_de2_resets[] = { > [RST_WB] = { 0x08, BIT(2) }, > }; > > +static struct ccu_reset_map sun50i_h6_de3_resets[] = { > + [RST_MIXER0] = { 0x08, BIT(0) }, > + [RST_MIXER1] = { 0x08, BIT(1) }, > + [RST_WB] = { 0x08, BIT(2) }, > + [RST_ROT] = { 0x08, BIT(3) }, > +}; > + > static const struct sunxi_ccu_desc sun8i_a83t_de2_clk_desc = { > .ccu_clks = sun8i_a83t_de2_clks, > .num_ccu_clks = ARRAY_SIZE(sun8i_a83t_de2_clks), > @@ -186,6 +237,16 @@ static const struct sunxi_ccu_desc sun50i_a64_de2_clk_desc = { > .num_resets = ARRAY_SIZE(sun50i_a64_de2_resets), > }; > > +static const struct sunxi_ccu_desc sun50i_h6_de3_clk_desc = { > + .ccu_clks = sun50i_h6_de3_clks, > + .num_ccu_clks = ARRAY_SIZE(sun50i_h6_de3_clks), > + > + .hw_clks = &sun50i_h6_de3_hw_clks, > + > + .resets = sun50i_h6_de3_resets, > + .num_resets = ARRAY_SIZE(sun50i_h6_de3_resets), > +}; > + > static const struct sunxi_ccu_desc sun8i_v3s_de2_clk_desc = { > .ccu_clks = sun8i_v3s_de2_clks, > .num_ccu_clks = ARRAY_SIZE(sun8i_v3s_de2_clks), > @@ -296,6 +357,10 @@ static const struct of_device_id sunxi_de2_clk_ids[] = { > .compatible = "allwinner,sun50i-h5-de2-clk", > .data = &sun50i_a64_de2_clk_desc, > }, > + { > + .compatible = "allwinner,sun50i-h6-de3-clk", > + .data = &sun50i_h6_de3_clk_desc, > + }, > { } > }; > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > index 530c006e0ae9..27bd88539f42 100644 > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > @@ -22,6 +22,7 @@ > #define CLK_MIXER0_DIV 3 > #define CLK_MIXER1_DIV 4 > #define CLK_WB_DIV 5 > +#define CLK_ROT_DIV 11 > > #define CLK_NUMBER (CLK_WB + 1) > > -- > 2.18.0 >
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Allwinner H6 SoC has multiplier N range between 1 and 254. Since parent > rate is 24MHz, intermediate result when calculating final rate easily > overflows 32 bit variable. > > Because of that, introduce function for calculating clock rate which > uses 64 bit variable for intermediate result. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> The code looks good. The A80's Video PLLs are also affected by this. The range for N on the A80 is 12 ~ 255. Can you add fixes and stable tags? ChenYu
Dne torek, 04. september 2018 ob 11:04:21 CEST je Chen-Yu Tsai napisal(a): > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Support for mixer0, mixer1, writeback and rotation units is added. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > --- > > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 65 ++++++++++++++++++++++++++++ > > drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 1 + > > 2 files changed, 66 insertions(+) > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c index bae5ee67a797..4535c1c27d27 > > 100644 > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > @@ -31,6 +31,8 @@ static SUNXI_CCU_GATE(bus_mixer1_clk, "bus-mixer1", > > "bus-de",> > > 0x04, BIT(1), 0); > > > > static SUNXI_CCU_GATE(bus_wb_clk, "bus-wb", "bus-de", > > > > 0x04, BIT(2), 0); > > > > +static SUNXI_CCU_GATE(bus_rot_clk, "bus-rot", "bus-de", > > + 0x04, BIT(3), 0); > > > > static SUNXI_CCU_GATE(mixer0_clk, "mixer0", "mixer0-div", > > > > 0x00, BIT(0), CLK_SET_RATE_PARENT); > > > > @@ -38,6 +40,8 @@ static SUNXI_CCU_GATE(mixer1_clk, "mixer1", > > "mixer1-div",> > > 0x00, BIT(1), CLK_SET_RATE_PARENT); > > > > static SUNXI_CCU_GATE(wb_clk, "wb", "wb-div", > > > > 0x00, BIT(2), CLK_SET_RATE_PARENT); > > > > +static SUNXI_CCU_GATE(rot_clk, "rot", "rot-div", > > + 0x00, BIT(3), CLK_SET_RATE_PARENT); > > > > static SUNXI_CCU_M(mixer0_div_clk, "mixer0-div", "de", 0x0c, 0, 4, > > > > CLK_SET_RATE_PARENT); > > > > @@ -45,6 +49,8 @@ static SUNXI_CCU_M(mixer1_div_clk, "mixer1-div", "de", > > 0x0c, 4, 4,> > > CLK_SET_RATE_PARENT); > > > > static SUNXI_CCU_M(wb_div_clk, "wb-div", "de", 0x0c, 8, 4, > > > > CLK_SET_RATE_PARENT); > > > > +static SUNXI_CCU_M(rot_div_clk, "rot-div", "de", 0x0c, 0x0c, 4, > > + CLK_SET_RATE_PARENT); > > > > static SUNXI_CCU_M(mixer0_div_a83_clk, "mixer0-div", "pll-de", 0x0c, 0, > > 4, > > > > CLK_SET_RATE_PARENT); > > > > @@ -53,6 +59,24 @@ static SUNXI_CCU_M(mixer1_div_a83_clk, "mixer1-div", > > "pll-de", 0x0c, 4, 4,> > > static SUNXI_CCU_M(wb_div_a83_clk, "wb-div", "pll-de", 0x0c, 8, 4, > > > > CLK_SET_RATE_PARENT); > > > > +static struct ccu_common *sun50i_h6_de3_clks[] = { > > + &mixer0_clk.common, > > + &mixer1_clk.common, > > + &wb_clk.common, > > + > > + &bus_mixer0_clk.common, > > + &bus_mixer1_clk.common, > > + &bus_wb_clk.common, > > + > > + &mixer0_div_clk.common, > > + &mixer1_div_clk.common, > > + &wb_div_clk.common, > > + > > + &bus_rot_clk.common, > > + &rot_clk.common, > > + &rot_div_clk.common, > > +}; > > + > > > > static struct ccu_common *sun8i_a83t_de2_clks[] = { > > > > &mixer0_clk.common, > > &mixer1_clk.common, > > > > @@ -92,6 +116,26 @@ static struct ccu_common *sun8i_v3s_de2_clks[] = { > > > > &wb_div_clk.common, > > > > }; > > > > +static struct clk_hw_onecell_data sun50i_h6_de3_hw_clks = { > > + .hws = { > > + [CLK_MIXER0] = &mixer0_clk.common.hw, > > + [CLK_MIXER1] = &mixer1_clk.common.hw, > > + [CLK_WB] = &wb_clk.common.hw, > > + [CLK_ROT] = &rot_clk.common.hw, > > + > > + [CLK_BUS_MIXER0] = &bus_mixer0_clk.common.hw, > > + [CLK_BUS_MIXER1] = &bus_mixer1_clk.common.hw, > > + [CLK_BUS_WB] = &bus_wb_clk.common.hw, > > + [CLK_BUS_ROT] = &bus_rot_clk.common.hw, > > + > > + [CLK_MIXER0_DIV] = &mixer0_div_clk.common.hw, > > + [CLK_MIXER1_DIV] = &mixer1_div_clk.common.hw, > > + [CLK_WB_DIV] = &wb_div_clk.common.hw, > > + [CLK_ROT_DIV] = &rot_div_clk.common.hw, > > + }, > > + .num = 12, > > It's best not to openly code these. It is error prone, like having > an index beyond .num, which then never gets registered. > > Instead, please update CLK_NUMBERS and use that instead. sunxi_ccu_probe() > can handle holes in .hws. I'm not sure this will work. All newly introduced indices are at the end, so other arrays will still have same length (hole at the end). You will just claim that arrays are larger than they really are, which means bad things. But I take any other suggestion. I really can't think of better solution. Best regards, Jernej > > On the other hand, it can't handle holes in the ccu_reset_map. Hope we > never have to deal with such an instance. > > ChenYu > > > +}; > > + > > > > static struct clk_hw_onecell_data sun8i_a83t_de2_hw_clks = { > > > > .hws = { > > > > [CLK_MIXER0] = &mixer0_clk.common.hw, > > > > @@ -156,6 +200,13 @@ static struct ccu_reset_map sun50i_a64_de2_resets[] = > > {> > > [RST_WB] = { 0x08, BIT(2) }, > > > > }; > > > > +static struct ccu_reset_map sun50i_h6_de3_resets[] = { > > + [RST_MIXER0] = { 0x08, BIT(0) }, > > + [RST_MIXER1] = { 0x08, BIT(1) }, > > + [RST_WB] = { 0x08, BIT(2) }, > > + [RST_ROT] = { 0x08, BIT(3) }, > > +}; > > + > > > > static const struct sunxi_ccu_desc sun8i_a83t_de2_clk_desc = { > > > > .ccu_clks = sun8i_a83t_de2_clks, > > .num_ccu_clks = ARRAY_SIZE(sun8i_a83t_de2_clks), > > > > @@ -186,6 +237,16 @@ static const struct sunxi_ccu_desc > > sun50i_a64_de2_clk_desc = {> > > .num_resets = ARRAY_SIZE(sun50i_a64_de2_resets), > > > > }; > > > > +static const struct sunxi_ccu_desc sun50i_h6_de3_clk_desc = { > > + .ccu_clks = sun50i_h6_de3_clks, > > + .num_ccu_clks = ARRAY_SIZE(sun50i_h6_de3_clks), > > + > > + .hw_clks = &sun50i_h6_de3_hw_clks, > > + > > + .resets = sun50i_h6_de3_resets, > > + .num_resets = ARRAY_SIZE(sun50i_h6_de3_resets), > > +}; > > + > > > > static const struct sunxi_ccu_desc sun8i_v3s_de2_clk_desc = { > > > > .ccu_clks = sun8i_v3s_de2_clks, > > .num_ccu_clks = ARRAY_SIZE(sun8i_v3s_de2_clks), > > > > @@ -296,6 +357,10 @@ static const struct of_device_id sunxi_de2_clk_ids[] > > = {> > > .compatible = "allwinner,sun50i-h5-de2-clk", > > .data = &sun50i_a64_de2_clk_desc, > > > > }, > > > > + { > > + .compatible = "allwinner,sun50i-h6-de3-clk", > > + .data = &sun50i_h6_de3_clk_desc, > > + }, > > > > { } > > > > }; > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h index 530c006e0ae9..27bd88539f42 > > 100644 > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > @@ -22,6 +22,7 @@ > > > > #define CLK_MIXER0_DIV 3 > > #define CLK_MIXER1_DIV 4 > > #define CLK_WB_DIV 5 > > > > +#define CLK_ROT_DIV 11 > > > > #define CLK_NUMBER (CLK_WB + 1) > > > > -- > > 2.18.0
Dne torek, 04. september 2018 ob 11:18:47 CEST je Chen-Yu Tsai napisal(a): > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Allwinner H6 SoC has multiplier N range between 1 and 254. Since parent > > rate is 24MHz, intermediate result when calculating final rate easily > > overflows 32 bit variable. > > > > Because of that, introduce function for calculating clock rate which > > uses 64 bit variable for intermediate result. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > The code looks good. The A80's Video PLLs are also affected by this. > The range for N on the A80 is 12 ~ 255. > > Can you add fixes and stable tags? Sure. > > ChenYu
On Wed, Sep 5, 2018 at 1:46 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne torek, 04. september 2018 ob 11:04:21 CEST je Chen-Yu Tsai napisal(a): > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > wrote: > > > Support for mixer0, mixer1, writeback and rotation units is added. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > --- > > > > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 65 ++++++++++++++++++++++++++++ > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 1 + > > > 2 files changed, 66 insertions(+) > > > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c index bae5ee67a797..4535c1c27d27 > > > 100644 > > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > @@ -31,6 +31,8 @@ static SUNXI_CCU_GATE(bus_mixer1_clk, "bus-mixer1", > > > "bus-de",> > > > 0x04, BIT(1), 0); > > > > > > static SUNXI_CCU_GATE(bus_wb_clk, "bus-wb", "bus-de", > > > > > > 0x04, BIT(2), 0); > > > > > > +static SUNXI_CCU_GATE(bus_rot_clk, "bus-rot", "bus-de", > > > + 0x04, BIT(3), 0); > > > > > > static SUNXI_CCU_GATE(mixer0_clk, "mixer0", "mixer0-div", > > > > > > 0x00, BIT(0), CLK_SET_RATE_PARENT); > > > > > > @@ -38,6 +40,8 @@ static SUNXI_CCU_GATE(mixer1_clk, "mixer1", > > > "mixer1-div",> > > > 0x00, BIT(1), CLK_SET_RATE_PARENT); > > > > > > static SUNXI_CCU_GATE(wb_clk, "wb", "wb-div", > > > > > > 0x00, BIT(2), CLK_SET_RATE_PARENT); > > > > > > +static SUNXI_CCU_GATE(rot_clk, "rot", "rot-div", > > > + 0x00, BIT(3), CLK_SET_RATE_PARENT); > > > > > > static SUNXI_CCU_M(mixer0_div_clk, "mixer0-div", "de", 0x0c, 0, 4, > > > > > > CLK_SET_RATE_PARENT); > > > > > > @@ -45,6 +49,8 @@ static SUNXI_CCU_M(mixer1_div_clk, "mixer1-div", "de", > > > 0x0c, 4, 4,> > > > CLK_SET_RATE_PARENT); > > > > > > static SUNXI_CCU_M(wb_div_clk, "wb-div", "de", 0x0c, 8, 4, > > > > > > CLK_SET_RATE_PARENT); > > > > > > +static SUNXI_CCU_M(rot_div_clk, "rot-div", "de", 0x0c, 0x0c, 4, > > > + CLK_SET_RATE_PARENT); > > > > > > static SUNXI_CCU_M(mixer0_div_a83_clk, "mixer0-div", "pll-de", 0x0c, 0, > > > 4, > > > > > > CLK_SET_RATE_PARENT); > > > > > > @@ -53,6 +59,24 @@ static SUNXI_CCU_M(mixer1_div_a83_clk, "mixer1-div", > > > "pll-de", 0x0c, 4, 4,> > > > static SUNXI_CCU_M(wb_div_a83_clk, "wb-div", "pll-de", 0x0c, 8, 4, > > > > > > CLK_SET_RATE_PARENT); > > > > > > +static struct ccu_common *sun50i_h6_de3_clks[] = { > > > + &mixer0_clk.common, > > > + &mixer1_clk.common, > > > + &wb_clk.common, > > > + > > > + &bus_mixer0_clk.common, > > > + &bus_mixer1_clk.common, > > > + &bus_wb_clk.common, > > > + > > > + &mixer0_div_clk.common, > > > + &mixer1_div_clk.common, > > > + &wb_div_clk.common, > > > + > > > + &bus_rot_clk.common, > > > + &rot_clk.common, > > > + &rot_div_clk.common, > > > +}; > > > + > > > > > > static struct ccu_common *sun8i_a83t_de2_clks[] = { > > > > > > &mixer0_clk.common, > > > &mixer1_clk.common, > > > > > > @@ -92,6 +116,26 @@ static struct ccu_common *sun8i_v3s_de2_clks[] = { > > > > > > &wb_div_clk.common, > > > > > > }; > > > > > > +static struct clk_hw_onecell_data sun50i_h6_de3_hw_clks = { > > > + .hws = { > > > + [CLK_MIXER0] = &mixer0_clk.common.hw, > > > + [CLK_MIXER1] = &mixer1_clk.common.hw, > > > + [CLK_WB] = &wb_clk.common.hw, > > > + [CLK_ROT] = &rot_clk.common.hw, > > > + > > > + [CLK_BUS_MIXER0] = &bus_mixer0_clk.common.hw, > > > + [CLK_BUS_MIXER1] = &bus_mixer1_clk.common.hw, > > > + [CLK_BUS_WB] = &bus_wb_clk.common.hw, > > > + [CLK_BUS_ROT] = &bus_rot_clk.common.hw, > > > + > > > + [CLK_MIXER0_DIV] = &mixer0_div_clk.common.hw, > > > + [CLK_MIXER1_DIV] = &mixer1_div_clk.common.hw, > > > + [CLK_WB_DIV] = &wb_div_clk.common.hw, > > > + [CLK_ROT_DIV] = &rot_div_clk.common.hw, > > > + }, > > > + .num = 12, > > > > It's best not to openly code these. It is error prone, like having > > an index beyond .num, which then never gets registered. > > > > Instead, please update CLK_NUMBERS and use that instead. sunxi_ccu_probe() > > can handle holes in .hws. > > I'm not sure this will work. All newly introduced indices are at the end, so > other arrays will still have same length (hole at the end). You will just > claim that arrays are larger than they really are, which means bad things. > > But I take any other suggestion. I really can't think of better solution. Then maybe have macros for both cases instead? CLK_NUMBER_WITH_ROT / CLK_NUMBER_WITHOUT_ROT? ChenYu > Best regards, > Jernej > > > > > On the other hand, it can't handle holes in the ccu_reset_map. Hope we > > never have to deal with such an instance. > > > > ChenYu > > > > > +}; > > > + > > > > > > static struct clk_hw_onecell_data sun8i_a83t_de2_hw_clks = { > > > > > > .hws = { > > > > > > [CLK_MIXER0] = &mixer0_clk.common.hw, > > > > > > @@ -156,6 +200,13 @@ static struct ccu_reset_map sun50i_a64_de2_resets[] = > > > {> > > > [RST_WB] = { 0x08, BIT(2) }, > > > > > > }; > > > > > > +static struct ccu_reset_map sun50i_h6_de3_resets[] = { > > > + [RST_MIXER0] = { 0x08, BIT(0) }, > > > + [RST_MIXER1] = { 0x08, BIT(1) }, > > > + [RST_WB] = { 0x08, BIT(2) }, > > > + [RST_ROT] = { 0x08, BIT(3) }, > > > +}; > > > + > > > > > > static const struct sunxi_ccu_desc sun8i_a83t_de2_clk_desc = { > > > > > > .ccu_clks = sun8i_a83t_de2_clks, > > > .num_ccu_clks = ARRAY_SIZE(sun8i_a83t_de2_clks), > > > > > > @@ -186,6 +237,16 @@ static const struct sunxi_ccu_desc > > > sun50i_a64_de2_clk_desc = {> > > > .num_resets = ARRAY_SIZE(sun50i_a64_de2_resets), > > > > > > }; > > > > > > +static const struct sunxi_ccu_desc sun50i_h6_de3_clk_desc = { > > > + .ccu_clks = sun50i_h6_de3_clks, > > > + .num_ccu_clks = ARRAY_SIZE(sun50i_h6_de3_clks), > > > + > > > + .hw_clks = &sun50i_h6_de3_hw_clks, > > > + > > > + .resets = sun50i_h6_de3_resets, > > > + .num_resets = ARRAY_SIZE(sun50i_h6_de3_resets), > > > +}; > > > + > > > > > > static const struct sunxi_ccu_desc sun8i_v3s_de2_clk_desc = { > > > > > > .ccu_clks = sun8i_v3s_de2_clks, > > > .num_ccu_clks = ARRAY_SIZE(sun8i_v3s_de2_clks), > > > > > > @@ -296,6 +357,10 @@ static const struct of_device_id sunxi_de2_clk_ids[] > > > = {> > > > .compatible = "allwinner,sun50i-h5-de2-clk", > > > .data = &sun50i_a64_de2_clk_desc, > > > > > > }, > > > > > > + { > > > + .compatible = "allwinner,sun50i-h6-de3-clk", > > > + .data = &sun50i_h6_de3_clk_desc, > > > + }, > > > > > > { } > > > > > > }; > > > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h index 530c006e0ae9..27bd88539f42 > > > 100644 > > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > > @@ -22,6 +22,7 @@ > > > > > > #define CLK_MIXER0_DIV 3 > > > #define CLK_MIXER1_DIV 4 > > > #define CLK_WB_DIV 5 > > > > > > +#define CLK_ROT_DIV 11 > > > > > > #define CLK_NUMBER (CLK_WB + 1) > > > > > > -- > > > 2.18.0 > > > >
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Currently supported Allwinner SoCs with DW HDMI controller have > scrambled addresses and read lock. However, that is not true in general. > For example, A80 and H6 have normal addresses and normal read access. > > Move code for unscrambling addresses and unlocking read access to it's > own function and call it from init function. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > H6 has DW HDMI 2.0 controller v2.12a. > > It supports 4K at 60 Hz and HDCP 2.2. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > index 16a0c7a88ea8..44143c9f20d0 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > @@ -43,6 +43,16 @@ sun8i_dw_hdmi_mode_valid_a83t(struct drm_connector *connector, > return MODE_OK; > } > > +static enum drm_mode_status > +sun8i_dw_hdmi_mode_valid_h6(struct drm_connector *connector, > + const struct drm_display_mode *mode) > +{ > + if (mode->clock > 600000) 600 MHz seems slightly arbitrary. AFAIK the dot clock for 4K 60Hz is 594 MHz? A comment on this limit would be nice. > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > + > static bool sun8i_dw_hdmi_node_is_tcon_top(struct device_node *node) > { > return IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP) && > @@ -220,12 +230,20 @@ static int sun8i_dw_hdmi_remove(struct platform_device *pdev) > return 0; > } > > +static const struct sun8i_dw_hdmi_quirks sun50i_h6_quirks = { > + .mode_valid = sun8i_dw_hdmi_mode_valid_h6, > +}; > + Please "version" sort the SoC families and models. > static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = { > .mode_valid = sun8i_dw_hdmi_mode_valid_a83t, > .set_rate = true, > }; > > static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = { > + { > + .compatible = "allwinner,sun50i-h6-dw-hdmi", > + .data = &sun50i_h6_quirks, > + }, Here also. Once fixed, Reviewed-by: Chen-Yu Tsai <wens@csie.org> > { > .compatible = "allwinner,sun8i-a83t-dw-hdmi", > .data = &sun8i_a83t_quirks, > -- > 2.18.0 >
On Tue, Sep 4, 2018 at 4:44 PM Icenowy Zheng <icenowy@aosc.io> wrote: > > > > 于 2018年9月4日 GMT+08:00 下午4:40:56, Chen-Yu Tsai <wens@csie.org> 写到: > >On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > >wrote: > >> > >> From: Icenowy Zheng <icenowy@aosc.io> > >> > >> As we have already binding for the H6 system controller, add its node > >> to the device tree. > >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > >> [fixed compatible string] > >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > >Reviewed-by: Chen-Yu Tsai <wens@csie.org> > > By the way can these patches be applied first? (patches > related to syscon). > > They're also needed for EMAC. Applied. BTW, the H6 system control EMAC register also has the internal PHY related controls seen on H3/H5. IIRC you previously mentioned that they have no effect? ChenYu
Dne sreda, 12. september 2018 ob 14:20:08 CEST je Chen-Yu Tsai napisal(a): > On Wed, Sep 5, 2018 at 1:46 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne torek, 04. september 2018 ob 11:04:21 CEST je Chen-Yu Tsai napisal(a): > > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > > > > wrote: > > > > Support for mixer0, mixer1, writeback and rotation units is added. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > --- > > > > > > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 65 > > > > ++++++++++++++++++++++++++++ > > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 1 + > > > > 2 files changed, 66 insertions(+) > > > > > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c index > > > > bae5ee67a797..4535c1c27d27 > > > > 100644 > > > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > > @@ -31,6 +31,8 @@ static SUNXI_CCU_GATE(bus_mixer1_clk, "bus-mixer1", > > > > "bus-de",> > > > > > > > > 0x04, BIT(1), 0); > > > > > > > > static SUNXI_CCU_GATE(bus_wb_clk, "bus-wb", "bus-de", > > > > > > > > 0x04, BIT(2), 0); > > > > > > > > +static SUNXI_CCU_GATE(bus_rot_clk, "bus-rot", "bus-de", > > > > + 0x04, BIT(3), 0); > > > > > > > > static SUNXI_CCU_GATE(mixer0_clk, "mixer0", "mixer0-div", > > > > > > > > 0x00, BIT(0), CLK_SET_RATE_PARENT); > > > > > > > > @@ -38,6 +40,8 @@ static SUNXI_CCU_GATE(mixer1_clk, "mixer1", > > > > "mixer1-div",> > > > > > > > > 0x00, BIT(1), CLK_SET_RATE_PARENT); > > > > > > > > static SUNXI_CCU_GATE(wb_clk, "wb", "wb-div", > > > > > > > > 0x00, BIT(2), CLK_SET_RATE_PARENT); > > > > > > > > +static SUNXI_CCU_GATE(rot_clk, "rot", "rot-div", > > > > + 0x00, BIT(3), CLK_SET_RATE_PARENT); > > > > > > > > static SUNXI_CCU_M(mixer0_div_clk, "mixer0-div", "de", 0x0c, 0, 4, > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > @@ -45,6 +49,8 @@ static SUNXI_CCU_M(mixer1_div_clk, "mixer1-div", > > > > "de", > > > > 0x0c, 4, 4,> > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > static SUNXI_CCU_M(wb_div_clk, "wb-div", "de", 0x0c, 8, 4, > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > +static SUNXI_CCU_M(rot_div_clk, "rot-div", "de", 0x0c, 0x0c, 4, > > > > + CLK_SET_RATE_PARENT); > > > > > > > > static SUNXI_CCU_M(mixer0_div_a83_clk, "mixer0-div", "pll-de", 0x0c, > > > > 0, > > > > 4, > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > @@ -53,6 +59,24 @@ static SUNXI_CCU_M(mixer1_div_a83_clk, > > > > "mixer1-div", > > > > "pll-de", 0x0c, 4, 4,> > > > > > > > > static SUNXI_CCU_M(wb_div_a83_clk, "wb-div", "pll-de", 0x0c, 8, 4, > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > +static struct ccu_common *sun50i_h6_de3_clks[] = { > > > > + &mixer0_clk.common, > > > > + &mixer1_clk.common, > > > > + &wb_clk.common, > > > > + > > > > + &bus_mixer0_clk.common, > > > > + &bus_mixer1_clk.common, > > > > + &bus_wb_clk.common, > > > > + > > > > + &mixer0_div_clk.common, > > > > + &mixer1_div_clk.common, > > > > + &wb_div_clk.common, > > > > + > > > > + &bus_rot_clk.common, > > > > + &rot_clk.common, > > > > + &rot_div_clk.common, > > > > +}; > > > > + > > > > > > > > static struct ccu_common *sun8i_a83t_de2_clks[] = { > > > > > > > > &mixer0_clk.common, > > > > &mixer1_clk.common, > > > > > > > > @@ -92,6 +116,26 @@ static struct ccu_common *sun8i_v3s_de2_clks[] = { > > > > > > > > &wb_div_clk.common, > > > > > > > > }; > > > > > > > > +static struct clk_hw_onecell_data sun50i_h6_de3_hw_clks = { > > > > + .hws = { > > > > + [CLK_MIXER0] = &mixer0_clk.common.hw, > > > > + [CLK_MIXER1] = &mixer1_clk.common.hw, > > > > + [CLK_WB] = &wb_clk.common.hw, > > > > + [CLK_ROT] = &rot_clk.common.hw, > > > > + > > > > + [CLK_BUS_MIXER0] = &bus_mixer0_clk.common.hw, > > > > + [CLK_BUS_MIXER1] = &bus_mixer1_clk.common.hw, > > > > + [CLK_BUS_WB] = &bus_wb_clk.common.hw, > > > > + [CLK_BUS_ROT] = &bus_rot_clk.common.hw, > > > > + > > > > + [CLK_MIXER0_DIV] = &mixer0_div_clk.common.hw, > > > > + [CLK_MIXER1_DIV] = &mixer1_div_clk.common.hw, > > > > + [CLK_WB_DIV] = &wb_div_clk.common.hw, > > > > + [CLK_ROT_DIV] = &rot_div_clk.common.hw, > > > > + }, > > > > + .num = 12, > > > > > > It's best not to openly code these. It is error prone, like having > > > an index beyond .num, which then never gets registered. > > > > > > Instead, please update CLK_NUMBERS and use that instead. > > > sunxi_ccu_probe() > > > can handle holes in .hws. > > > > I'm not sure this will work. All newly introduced indices are at the end, > > so other arrays will still have same length (hole at the end). You will > > just claim that arrays are larger than they really are, which means bad > > things. > > > > But I take any other suggestion. I really can't think of better solution. > > Then maybe have macros for both cases instead? > CLK_NUMBER_WITH_ROT / CLK_NUMBER_WITHOUT_ROT? That sounds reasonable. Do you want separate patch which renames original macro CLK_NUMBER to CLK_NUMBER_WITHOUT_ROT? Best regards, Jernej > > ChenYu > > > Best regards, > > Jernej > > > > > On the other hand, it can't handle holes in the ccu_reset_map. Hope we > > > never have to deal with such an instance. > > > > > > ChenYu > > > > > > > +}; > > > > + > > > > > > > > static struct clk_hw_onecell_data sun8i_a83t_de2_hw_clks = { > > > > > > > > .hws = { > > > > > > > > [CLK_MIXER0] = &mixer0_clk.common.hw, > > > > > > > > @@ -156,6 +200,13 @@ static struct ccu_reset_map > > > > sun50i_a64_de2_resets[] = > > > > {> > > > > > > > > [RST_WB] = { 0x08, BIT(2) }, > > > > > > > > }; > > > > > > > > +static struct ccu_reset_map sun50i_h6_de3_resets[] = { > > > > + [RST_MIXER0] = { 0x08, BIT(0) }, > > > > + [RST_MIXER1] = { 0x08, BIT(1) }, > > > > + [RST_WB] = { 0x08, BIT(2) }, > > > > + [RST_ROT] = { 0x08, BIT(3) }, > > > > +}; > > > > + > > > > > > > > static const struct sunxi_ccu_desc sun8i_a83t_de2_clk_desc = { > > > > > > > > .ccu_clks = sun8i_a83t_de2_clks, > > > > .num_ccu_clks = ARRAY_SIZE(sun8i_a83t_de2_clks), > > > > > > > > @@ -186,6 +237,16 @@ static const struct sunxi_ccu_desc > > > > sun50i_a64_de2_clk_desc = {> > > > > > > > > .num_resets = ARRAY_SIZE(sun50i_a64_de2_resets), > > > > > > > > }; > > > > > > > > +static const struct sunxi_ccu_desc sun50i_h6_de3_clk_desc = { > > > > + .ccu_clks = sun50i_h6_de3_clks, > > > > + .num_ccu_clks = ARRAY_SIZE(sun50i_h6_de3_clks), > > > > + > > > > + .hw_clks = &sun50i_h6_de3_hw_clks, > > > > + > > > > + .resets = sun50i_h6_de3_resets, > > > > + .num_resets = ARRAY_SIZE(sun50i_h6_de3_resets), > > > > +}; > > > > + > > > > > > > > static const struct sunxi_ccu_desc sun8i_v3s_de2_clk_desc = { > > > > > > > > .ccu_clks = sun8i_v3s_de2_clks, > > > > .num_ccu_clks = ARRAY_SIZE(sun8i_v3s_de2_clks), > > > > > > > > @@ -296,6 +357,10 @@ static const struct of_device_id > > > > sunxi_de2_clk_ids[] > > > > = {> > > > > > > > > .compatible = "allwinner,sun50i-h5-de2-clk", > > > > .data = &sun50i_a64_de2_clk_desc, > > > > > > > > }, > > > > > > > > + { > > > > + .compatible = "allwinner,sun50i-h6-de3-clk", > > > > + .data = &sun50i_h6_de3_clk_desc, > > > > + }, > > > > > > > > { } > > > > > > > > }; > > > > > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h index > > > > 530c006e0ae9..27bd88539f42 > > > > 100644 > > > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.h > > > > @@ -22,6 +22,7 @@ > > > > > > > > #define CLK_MIXER0_DIV 3 > > > > #define CLK_MIXER1_DIV 4 > > > > #define CLK_WB_DIV 5 > > > > > > > > +#define CLK_ROT_DIV 11 > > > > > > > > #define CLK_NUMBER (CLK_WB + 1) > > > > > > > > -- > > > > 2.18.0
于 2018年9月12日 GMT+08:00 下午10:49:12, Chen-Yu Tsai <wens@csie.org> 写到: >On Tue, Sep 4, 2018 at 4:44 PM Icenowy Zheng <icenowy@aosc.io> wrote: >> >> >> >> 于 2018年9月4日 GMT+08:00 下午4:40:56, Chen-Yu Tsai <wens@csie.org> 写到: >> >On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec ><jernej.skrabec@siol.net> >> >wrote: >> >> >> >> From: Icenowy Zheng <icenowy@aosc.io> >> >> >> >> As we have already binding for the H6 system controller, add its >node >> >> to the device tree. >> >> >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> >> [fixed compatible string] >> >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> >> > >> >Reviewed-by: Chen-Yu Tsai <wens@csie.org> >> >> By the way can these patches be applied first? (patches >> related to syscon). >> >> They're also needed for EMAC. > >Applied. > >BTW, the H6 system control EMAC register also has the internal PHY >related controls seen on H3/H5. IIRC you previously mentioned that >they have no effect? Yes, because the "internal PHY" on H6 is on AC200. The internal/external mux bit has effect (although it will mux to a non-existent PHY when switching to "internal", render EMAC not usable), but it also has effect on A64 (same effect with H6), on which it's not documented. > >ChenYu > >_______________________________________________ >linux-arm-kernel mailing list >linux-arm-kernel@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Sep 12, 2018 at 10:55 PM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne sreda, 12. september 2018 ob 14:20:08 CEST je Chen-Yu Tsai napisal(a): > > On Wed, Sep 5, 2018 at 1:46 AM Jernej Škrabec <jernej.skrabec@siol.net> > wrote: > > > Dne torek, 04. september 2018 ob 11:04:21 CEST je Chen-Yu Tsai napisal(a): > > > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > > > > > > wrote: > > > > > Support for mixer0, mixer1, writeback and rotation units is added. > > > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > --- > > > > > > > > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 65 > > > > > ++++++++++++++++++++++++++++ > > > > > drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 1 + > > > > > 2 files changed, 66 insertions(+) > > > > > > > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > > > b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c index > > > > > bae5ee67a797..4535c1c27d27 > > > > > 100644 > > > > > --- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > > > +++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c > > > > > @@ -31,6 +31,8 @@ static SUNXI_CCU_GATE(bus_mixer1_clk, "bus-mixer1", > > > > > "bus-de",> > > > > > > > > > > 0x04, BIT(1), 0); > > > > > > > > > > static SUNXI_CCU_GATE(bus_wb_clk, "bus-wb", "bus-de", > > > > > > > > > > 0x04, BIT(2), 0); > > > > > > > > > > +static SUNXI_CCU_GATE(bus_rot_clk, "bus-rot", "bus-de", > > > > > + 0x04, BIT(3), 0); > > > > > > > > > > static SUNXI_CCU_GATE(mixer0_clk, "mixer0", "mixer0-div", > > > > > > > > > > 0x00, BIT(0), CLK_SET_RATE_PARENT); > > > > > > > > > > @@ -38,6 +40,8 @@ static SUNXI_CCU_GATE(mixer1_clk, "mixer1", > > > > > "mixer1-div",> > > > > > > > > > > 0x00, BIT(1), CLK_SET_RATE_PARENT); > > > > > > > > > > static SUNXI_CCU_GATE(wb_clk, "wb", "wb-div", > > > > > > > > > > 0x00, BIT(2), CLK_SET_RATE_PARENT); > > > > > > > > > > +static SUNXI_CCU_GATE(rot_clk, "rot", "rot-div", > > > > > + 0x00, BIT(3), CLK_SET_RATE_PARENT); > > > > > > > > > > static SUNXI_CCU_M(mixer0_div_clk, "mixer0-div", "de", 0x0c, 0, 4, > > > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > > > @@ -45,6 +49,8 @@ static SUNXI_CCU_M(mixer1_div_clk, "mixer1-div", > > > > > "de", > > > > > 0x0c, 4, 4,> > > > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > > > static SUNXI_CCU_M(wb_div_clk, "wb-div", "de", 0x0c, 8, 4, > > > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > > > +static SUNXI_CCU_M(rot_div_clk, "rot-div", "de", 0x0c, 0x0c, 4, > > > > > + CLK_SET_RATE_PARENT); > > > > > > > > > > static SUNXI_CCU_M(mixer0_div_a83_clk, "mixer0-div", "pll-de", 0x0c, > > > > > 0, > > > > > 4, > > > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > > > @@ -53,6 +59,24 @@ static SUNXI_CCU_M(mixer1_div_a83_clk, > > > > > "mixer1-div", > > > > > "pll-de", 0x0c, 4, 4,> > > > > > > > > > > static SUNXI_CCU_M(wb_div_a83_clk, "wb-div", "pll-de", 0x0c, 8, 4, > > > > > > > > > > CLK_SET_RATE_PARENT); > > > > > > > > > > +static struct ccu_common *sun50i_h6_de3_clks[] = { > > > > > + &mixer0_clk.common, > > > > > + &mixer1_clk.common, > > > > > + &wb_clk.common, > > > > > + > > > > > + &bus_mixer0_clk.common, > > > > > + &bus_mixer1_clk.common, > > > > > + &bus_wb_clk.common, > > > > > + > > > > > + &mixer0_div_clk.common, > > > > > + &mixer1_div_clk.common, > > > > > + &wb_div_clk.common, > > > > > + > > > > > + &bus_rot_clk.common, > > > > > + &rot_clk.common, > > > > > + &rot_div_clk.common, > > > > > +}; > > > > > + > > > > > > > > > > static struct ccu_common *sun8i_a83t_de2_clks[] = { > > > > > > > > > > &mixer0_clk.common, > > > > > &mixer1_clk.common, > > > > > > > > > > @@ -92,6 +116,26 @@ static struct ccu_common *sun8i_v3s_de2_clks[] = { > > > > > > > > > > &wb_div_clk.common, > > > > > > > > > > }; > > > > > > > > > > +static struct clk_hw_onecell_data sun50i_h6_de3_hw_clks = { > > > > > + .hws = { > > > > > + [CLK_MIXER0] = &mixer0_clk.common.hw, > > > > > + [CLK_MIXER1] = &mixer1_clk.common.hw, > > > > > + [CLK_WB] = &wb_clk.common.hw, > > > > > + [CLK_ROT] = &rot_clk.common.hw, > > > > > + > > > > > + [CLK_BUS_MIXER0] = &bus_mixer0_clk.common.hw, > > > > > + [CLK_BUS_MIXER1] = &bus_mixer1_clk.common.hw, > > > > > + [CLK_BUS_WB] = &bus_wb_clk.common.hw, > > > > > + [CLK_BUS_ROT] = &bus_rot_clk.common.hw, > > > > > + > > > > > + [CLK_MIXER0_DIV] = &mixer0_div_clk.common.hw, > > > > > + [CLK_MIXER1_DIV] = &mixer1_div_clk.common.hw, > > > > > + [CLK_WB_DIV] = &wb_div_clk.common.hw, > > > > > + [CLK_ROT_DIV] = &rot_div_clk.common.hw, > > > > > + }, > > > > > + .num = 12, > > > > > > > > It's best not to openly code these. It is error prone, like having > > > > an index beyond .num, which then never gets registered. > > > > > > > > Instead, please update CLK_NUMBERS and use that instead. > > > > sunxi_ccu_probe() > > > > can handle holes in .hws. > > > > > > I'm not sure this will work. All newly introduced indices are at the end, > > > so other arrays will still have same length (hole at the end). You will > > > just claim that arrays are larger than they really are, which means bad > > > things. > > > > > > But I take any other suggestion. I really can't think of better solution. > > > > Then maybe have macros for both cases instead? > > CLK_NUMBER_WITH_ROT / CLK_NUMBER_WITHOUT_ROT? > > That sounds reasonable. Do you want separate patch which renames original > macro CLK_NUMBER to CLK_NUMBER_WITHOUT_ROT? I don't think that's necessary. Just mention the name change in the changelog, and justifying it by the addition of the ROT clocks. Thanks
Hi, On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Most, if not all, registers found in DE2 still exists in DE3. However, > units are on different base addresses. > > To prepare for addition of DE3 support, registers macros are reworked so > they take base address as parameter. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > [rebased] > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> This patch mostly checks out. But see below. > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h > index 406c42e752d7..020b0a097c84 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > @@ -29,20 +29,24 @@ > > #define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE BIT(0) > > -#define SUN8I_MIXER_BLEND_PIPE_CTL 0x1000 > -#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(x) (0x1004 + 0x10 * (x) + 0x0) > -#define SUN8I_MIXER_BLEND_ATTR_INSIZE(x) (0x1004 + 0x10 * (x) + 0x4) > -#define SUN8I_MIXER_BLEND_ATTR_COORD(x) (0x1004 + 0x10 * (x) + 0x8) > -#define SUN8I_MIXER_BLEND_ROUTE 0x1080 > -#define SUN8I_MIXER_BLEND_PREMULTIPLY 0x1084 > -#define SUN8I_MIXER_BLEND_BKCOLOR 0x1088 > -#define SUN8I_MIXER_BLEND_OUTSIZE 0x108c > -#define SUN8I_MIXER_BLEND_MODE(x) (0x1090 + 0x04 * (x)) > -#define SUN8I_MIXER_BLEND_CK_CTL 0x10b0 > -#define SUN8I_MIXER_BLEND_CK_CFG 0x10b4 > -#define SUN8I_MIXER_BLEND_CK_MAX(x) (0x10c0 + 0x04 * (x)) > -#define SUN8I_MIXER_BLEND_CK_MIN(x) (0x10e0 + 0x04 * (x)) > -#define SUN8I_MIXER_BLEND_OUTCTL 0x10fc > +#define DE2_BLD_BASE 0x1000 > +#define DE2_CH_BASE 0x2000 > +#define DE2_CH_SIZE 0x1000 > + > +#define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * (x)) > +#define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * (x)) > +#define SUN8I_MIXER_BLEND_ATTR_COORD(base, x) ((base) + 0xC + 0x10 * (x)) Nit: Use lowercase for '0xC' to be consistent. > +#define SUN8I_MIXER_BLEND_ROUTE(base) ((base) + 0x80) > +#define SUN8I_MIXER_BLEND_PREMULTIPLY(base) ((base) + 0x84) > +#define SUN8I_MIXER_BLEND_BKCOLOR(base) ((base) + 0x88) > +#define SUN8I_MIXER_BLEND_OUTSIZE(base) ((base) + 0x8c) > +#define SUN8I_MIXER_BLEND_MODE(base, x) ((base) + 0x90 + 0x04 * (x)) > +#define SUN8I_MIXER_BLEND_CK_CTL(base) ((base) + 0xb0) > +#define SUN8I_MIXER_BLEND_CK_CFG(base) ((base) + 0xb4) > +#define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * (x)) > +#define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + 0x04 * (x)) > +#define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + 0xfc) > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) [...] > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c b/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > index 6bb2aa164c8e..c68eab8a748f 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > @@ -10,6 +10,7 @@ > */ > > #include "sun8i_ui_scaler.h" > +#include "sun8i_vi_scaler.h" > > static const u32 lan2coefftab16[240] = { > 0x00004000, 0x00033ffe, 0x00063efc, 0x000a3bfb, > @@ -88,6 +89,14 @@ static const u32 lan2coefftab16[240] = { > 0x0b1c1603, 0x0d1c1502, 0x0e1d1401, 0x0f1d1301, > }; > > +static inline u32 sun8i_ui_scaler_base(struct sun8i_mixer *mixer, int channel) I recently saw a review comment stating one should not inline functions unless they are defined in header files. Otherwise the decision should be left up to the compiler. > +{ > + int vi_num = mixer->cfg->vi_num; > + > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * vi_num + > + DE2_UI_SCALER_SIZE * (channel - vi_num); > +} > + > static int sun8i_ui_scaler_coef_index(unsigned int step) > { > unsigned int scale, int_part, float_part; [...] > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > index d3f1acb234b7..8697afc36023 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > @@ -833,6 +833,11 @@ static const u32 bicubic4coefftab32[480] = { > 0x1012110d, 0x1012110d, 0x1013110c, 0x1013110c, > }; > > +static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int channel) > +{ > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > +} > + This one as well. > static int sun8i_vi_scaler_coef_index(unsigned int step) > { > unsigned int scale, int_part, float_part; > @@ -857,7 +862,7 @@ static int sun8i_vi_scaler_coef_index(unsigned int step) > } > } > > -static void sun8i_vi_scaler_set_coeff(struct regmap *map, int layer, > +static void sun8i_vi_scaler_set_coeff(struct regmap *map, u32 base, > u32 hstep, u32 vstep, > const struct drm_format_info *format) This is the only instance where a function's "layer" parameter was changed to "base". It would be nice if it were consistent. ChenYu
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Display Engine 3 is an upgrade of DE2 with new features like support for > 10 bit color formats and support for AFBC. > > Most of DE2 code works with DE3, except some small details. > > Add support for it. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_csc.c | 96 +++++++++++++++++++++++-- > drivers/gpu/drm/sun4i/sun8i_mixer.c | 17 ++++- > drivers/gpu/drm/sun4i/sun8i_mixer.h | 21 +++++- > drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 8 ++- > drivers/gpu/drm/sun4i/sun8i_ui_scaler.h | 1 + > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 8 +++ > drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 2 + > drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++- > drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++ > 9 files changed, 197 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c b/drivers/gpu/drm/sun4i/sun8i_csc.c > index b14925b40ccf..101901ccf2dc 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = { > 0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A, > }; > > +/* > + * DE3 has a bit different CSC units. Factors are in two's complement format. > + * First three have 17 bits for fractinal part and last two 2 bits. First > + * three values in each line are multiplication factor, 4th is difference, > + * which is subtracted from the input value before the multiplication and > + * last value is constant, which is added at the end. > + * > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0 > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1 > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2 > + * > + * Please note that above formula is true only for Blender CSC. Other DE3 CSC > + * units takes only positive value for difference. From what can be deducted > + * from BSP driver code, those units probably automatically assume that > + * difference has to be subtracted. > + */ > +static const u32 yuv2rgb_de3[] = { > + 0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000, > + 0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000, > + 0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000, > +}; > + > +static const u32 yvu2rgb_de3[] = { > + 0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000, > + 0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000, > + 0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000, > +}; > + > static void sun8i_csc_set_coefficients(struct regmap *map, u32 base, > enum sun8i_csc_mode mode) > { > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap *map, u32 base, > } > } > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int layer, > + enum sun8i_csc_mode mode) > +{ > + const u32 *table; > + int i, j; > + > + switch (mode) { > + case SUN8I_CSC_MODE_YUV2RGB: > + table = yuv2rgb_de3; > + break; > + case SUN8I_CSC_MODE_YVU2RGB: > + table = yvu2rgb_de3; > + break; > + default: > + DRM_WARN("Wrong CSC mode specified.\n"); > + return; > + } > + > + for (i = 0; i < 3; i++) { > + for (j = 0; j < 3; j++) > + regmap_write(map, > + SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, > + layer, i, j), > + table[i * 5 + j]); Given that the first three values occupy contiguous addresses, you can use regmap_bulk_write() here. > + regmap_write(map, > + SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE, > + layer, i), > + SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 + 3], > + table[i * 5 + 4])); Nit: Using a two-dimension array might make it easier to read. > + } > +} > + > static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable) > { > u32 val; > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable) > regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN, val); > } > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool enable) > +{ > + u32 val, mask; > + > + mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer); > + > + if (enable) > + val = mask; > + else > + val = 0; > + > + regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE), > + mask, val); > +} > + > void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int layer, > enum sun8i_csc_mode mode) > { > - u32 base; > + if (!mixer->cfg->is_de3) { > + u32 base; You could rewrite this as if (mixer->cfg->is_de3) { sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, layer, mode); return; } That way you don't need to change the indentation on the existing lines. I suppose this is more of a personal preference. The downside is the control flow is slightly more complicated. > - base = ccsc_base[mixer->cfg->ccsc][layer]; > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > - sun8i_csc_set_coefficients(mixer->engine.regs, base, mode); > + sun8i_csc_set_coefficients(mixer->engine.regs, base, mode); > + } else { > + sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > + layer, mode); > + } > } > > void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool enable) > { > - u32 base; > + if (!mixer->cfg->is_de3) { > + u32 base; > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > - sun8i_csc_enable(mixer->engine.regs, base, enable); > + sun8i_csc_enable(mixer->engine.regs, base, enable); > + } else { > + sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable); > + } > } > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c > index 743941a33d88..a9218abf0935 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master, > base = sun8i_blender_base(mixer); > > /* Reset the registers */ > - for (i = 0x0; i < 0x20000; i += 4) > - regmap_write(mixer->engine.regs, i, 0); > + if (mixer->cfg->is_de3) { > + for (i = 0x0; i < 0x3000; i += 4) > + regmap_write(mixer->engine.regs, i, 0); > + for (i = 0x20000; i < 0x40000; i += 4) > + regmap_write(mixer->engine.regs, i, 0); > + for (i = 0x70000; i < 0x88000; i += 4) > + regmap_write(mixer->engine.regs, i, 0); > + for (i = 0xa0000; i < 0xb0000; i += 4) > + regmap_write(mixer->engine.regs, i, 0); > + for (i = 0xd0000; i < 0xe0000; i += 4) > + regmap_write(mixer->engine.regs, i, 0); Could you use the macros *_BASE and *_SIZE here? That would make it more obviously what parts you're clearing. The last offset, 0xd0000, isn't even defined in the DE3 spec. > + } else { > + for (i = 0x0; i < 0x20000; i += 4) > + regmap_write(mixer->engine.regs, i, 0); > + } > > /* Enable the mixer */ > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL, > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h > index 020b0a097c84..4c9a442bbb44 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > @@ -33,6 +33,10 @@ > #define DE2_CH_BASE 0x2000 > #define DE2_CH_SIZE 0x1000 > > +#define DE3_BLD_BASE 0x0800 > +#define DE3_CH_BASE 0x1000 > +#define DE3_CH_SIZE 0x0800 > + > #define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * (x)) > #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * (x)) > @@ -47,6 +51,11 @@ > #define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * (x)) > #define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + 0x04 * (x)) > #define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + 0xfc) > +#define SUN8I_MIXER_BLEND_CSC_CTL(base) ((base) + 0x100) > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \ > + ((base) + 0x110 + (layer) * 0x30 + (x) * 0x10 + 4 * (y)) > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \ > + ((base) + 0x110 + (layer) * 0x30 + (i) * 0x10 + 0x0c) Should these have a DE3 prefix? > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > @@ -61,6 +70,9 @@ > > #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED BIT(1) > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch) BIT(ch) > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c) (((d) << 16) | ((c) & 0xffff)) > + Same here. > #define SUN8I_MIXER_FBFMT_ARGB8888 0 > #define SUN8I_MIXER_FBFMT_ABGR8888 1 > #define SUN8I_MIXER_FBFMT_RGBA8888 2 [...] > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > index 46f0237c17bb..3d0ad64178ea 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > @@ -30,6 +30,8 @@ > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE BIT(15) > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET 8 > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK GENMASK(12, 8) > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK GENMASK(31, 24) > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x) ((x) << 24) DE3 prefix? > > struct sun8i_mixer; > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > index 8697afc36023..22e1ed5cd7a4 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = { > > static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int channel) > { > - return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > + if (mixer->cfg->is_de3) > + return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel; > + else > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > } > > static int sun8i_vi_scaler_coef_index(unsigned int step) > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int layer, > cvphase = vphase; > } > > + if (mixer->cfg->is_de3) { > + u32 val; > + > + if (format->hsub == 1 && format->vsub == 1) > + val = SUN8I_SCALER_VSU_SCALE_MODE_UI; > + else > + val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL; > + > + regmap_write(mixer->engine.regs, > + SUN8I_SCALER_VSU_SCALE_MODE(base), val); The remaining settings seem to be related to the edge detection scaling method. Since you aren't supporting it, are they necessary? > + regmap_write(mixer->engine.regs, > + SUN8I_SCALER_VSU_DIR_THR(base), > + SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) | > + SUN8I_SCALER_VSU_ZERO_DIR_THR(0) | > + SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) | > + SUN8I_SCALER_VSU_VERT_DIR_THR(1)); > + regmap_write(mixer->engine.regs, > + SUN8I_SCALER_VSU_EDGE_THR(base), > + SUN8I_SCALER_VSU_EDGE_SHIFT(8) | > + SUN8I_SCALER_VSU_EDGE_OFFSET(0)); > + regmap_write(mixer->engine.regs, > + SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0); > + regmap_write(mixer->engine.regs, > + SUN8I_SCALER_VSU_ANGLE_THR(base), > + SUN8I_SCALER_VSU_ANGLE_SHIFT(2) | > + SUN8I_SCALER_VSU_ANGLE_OFFSET(0)); > + } > + > regmap_write(mixer->engine.regs, > SUN8I_SCALER_VSU_OUTSIZE(base), outsize); > regmap_write(mixer->engine.regs, > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > index cd015405f66d..c322f5652481 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > @@ -15,6 +15,9 @@ > #define DE2_VI_SCALER_BASE 0x20000 > #define DE2_VI_SCALER_SIZE 0x20000 > > +#define DE3_VI_SCALER_BASE 0x20000 > +#define DE3_VI_SCALER_SIZE 0x08000 > + > /* this two macros assumes 16 fractional bits which is standard in DRM */ > #define SUN8I_VI_SCALER_SCALE_MIN 1 > #define SUN8I_VI_SCALER_SCALE_MAX ((1UL << 20) - 1) > @@ -25,6 +28,11 @@ > #define SUN8I_VI_SCALER_SIZE(w, h) (((h) - 1) << 16 | ((w) - 1)) > > #define SUN8I_SCALER_VSU_CTRL(base) ((base) + 0x0) > +#define SUN8I_SCALER_VSU_SCALE_MODE(base) ((base) + 0x10) > +#define SUN8I_SCALER_VSU_DIR_THR(base) ((base) + 0x20) > +#define SUN8I_SCALER_VSU_EDGE_THR(base) ((base) + 0x24) > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base) ((base) + 0x28) > +#define SUN8I_SCALER_VSU_ANGLE_THR(base) ((base) + 0x2c) DE3 prefix for the new ones please. > #define SUN8I_SCALER_VSU_OUTSIZE(base) ((base) + 0x40) > #define SUN8I_SCALER_VSU_YINSIZE(base) ((base) + 0x80) > #define SUN8I_SCALER_VSU_YHSTEP(base) ((base) + 0x88) > @@ -46,6 +54,21 @@ > #define SUN8I_SCALER_VSU_CTRL_EN BIT(0) > #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY BIT(4) > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x) (((x) << 24) & 0xFF) > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x) (((x) << 16) & 0xFF) > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x) (((x) << 8) & 0xFF) > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x) ((x) & 0xFF) > + > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI 0 > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL 1 > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE 2 > + > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x) (((x) << 16) & 0xF) > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x) ((x) & 0xFF) > + > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x) (((x) << 16) & 0xF) > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x) ((x) & 0xFF) > + Same here. Regards ChenYu > void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool enable); > void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int layer, > u32 src_w, u32 src_h, u32 dst_w, u32 dst_h, > -- > 2.18.0 >
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Mixer 0 has 1 VI and 3 UI planes, scaler on all planes and can output > 4K image @60Hz. It also support 10 bit colors. AFAICT 10 bit color support is not implemented? Please mention this. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_mixer.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c > index a9218abf0935..54eca2dd4b33 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > @@ -540,6 +540,15 @@ static int sun8i_mixer_remove(struct platform_device *pdev) > return 0; > } > > +static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = { Please sort the per-compatible structures according to "version sort" rules. > + .ccsc = 0, > + .is_de3 = true, > + .mod_rate = 600000000, > + .scaler_mask = 0xf, > + .ui_num = 3, > + .vi_num = 1, > +}; > + > static const struct sun8i_mixer_cfg sun8i_a83t_mixer0_cfg = { > .ccsc = 0, > .scaler_mask = 0xf, > @@ -587,6 +596,10 @@ static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = { > }; > > static const struct of_device_id sun8i_mixer_of_table[] = { > + { > + .compatible = "allwinner,sun50i-h6-de3-mixer-0", > + .data = &sun50i_h6_mixer0_cfg, > + }, Same here. ChenYu > { > .compatible = "allwinner,sun8i-a83t-de2-mixer-0", > .data = &sun8i_a83t_mixer0_cfg, > -- > 2.18.0 >
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Since it is not possible to access sun8i-dw-hdmi driver private data > inside mode_valid function, make it configurable. That way different > versions of HDMI controllers can set different function, depending on > it's limitations. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > It turns out that H6 HDMI BSP kernel driver doesn't change TMDS rate at > all. At this point it is not clear whether it is just not necessary or > it would cause some kind of issues. > > Add a quirk for it. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Sat, Sep 22, 2018 at 9:23 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > > > Mixer 0 has 1 VI and 3 UI planes, scaler on all planes and can output > > 4K image @60Hz. It also support 10 bit colors. > > AFAICT 10 bit color support is not implemented? Please mention this. > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > index a9218abf0935..54eca2dd4b33 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > @@ -540,6 +540,15 @@ static int sun8i_mixer_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = { > > Please sort the per-compatible structures according to "version sort" rules. > > > + .ccsc = 0, > > + .is_de3 = true, > > + .mod_rate = 600000000, > > + .scaler_mask = 0xf, > > + .ui_num = 3, > > + .vi_num = 1, > > +}; > > + > > static const struct sun8i_mixer_cfg sun8i_a83t_mixer0_cfg = { > > .ccsc = 0, > > .scaler_mask = 0xf, > > @@ -587,6 +596,10 @@ static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = { > > }; > > > > static const struct of_device_id sun8i_mixer_of_table[] = { > > + { > > + .compatible = "allwinner,sun50i-h6-de3-mixer-0", > > + .data = &sun50i_h6_mixer0_cfg, > > + }, > > Same here. > > ChenYu BTW, DE 3.0 includes a register in DE TOP called "DE IP configure register", which gives the number of IP blocks per class, per mixer. If we retrieve the configuration from this register, then we shouldn't need to differentiate between mixer-0 and mixer-1 with compatible strings. What do you think? ChenYu
On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > It turns out that even new DW HDMI controllers exhibits same mangenta magenta? Otherwise, FWIW, Reviewed-by: Chen-Yu Tsai <wens@csie.org> > line issues as older versions. > > Enable workaround for v2.12a. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 5971976284bf..df1c7a2d6961 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -1664,6 +1664,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi) > case 0x131a: > case 0x132a: > case 0x201a: > + case 0x212a: > count = 1; > break; > default: > -- > 2.18.0 >
On Sun, Sep 2, 2018 at 3:28 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > H6 has Synopsys DWC HDMI 2.0 TX PHY. > > mpll settings were calculated from specifications of similar Synopsys > HDMI PHY found in i.MX6. Other PHY settings were derived from BSP PHY > driver code. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 137 +++++++++++++++++++++++++ > 1 file changed, 137 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > index ee2bf61cd4d2..2f5499bd35ec 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > @@ -14,6 +14,122 @@ > */ > #define I2C_ADDR 0x69 > > +static const struct dw_hdmi_mpll_config sun50i_h6_mpll_cfg[] = { How did you choose the pixel clock points for this table? The values sort of match up with the BSP. > + { > + 30666000, { > + { 0x00b3, 0x0000 }, > + { 0x2153, 0x0000 }, > + { 0x40f3, 0x0000 }, > + }, > + }, { > + 36800000, { > + { 0x00b3, 0x0000 }, > + { 0x2153, 0x0000 }, > + { 0x40a2, 0x0001 }, > + }, > + }, { > + 46000000, { > + { 0x00b3, 0x0000 }, > + { 0x2142, 0x0001 }, > + { 0x40a2, 0x0001 }, > + }, > + }, { > + 61333000, { > + { 0x0072, 0x0001 }, > + { 0x2142, 0x0001 }, > + { 0x40a2, 0x0001 }, > + }, > + }, { > + 73600000, { > + { 0x0072, 0x0001 }, > + { 0x2142, 0x0001 }, > + { 0x4061, 0x0002 }, > + }, > + }, { > + 92000000, { > + { 0x0072, 0x0001 }, > + { 0x2145, 0x0002 }, > + { 0x4061, 0x0002 }, > + }, > + }, { > + 122666000, { > + { 0x0051, 0x0002 }, > + { 0x2145, 0x0002 }, > + { 0x4061, 0x0002 }, > + }, > + }, { > + 147200000, { > + { 0x0051, 0x0002 }, > + { 0x2145, 0x0002 }, > + { 0x4064, 0x0003 }, > + }, > + }, { > + 184000000, { > + { 0x0051, 0x0002 }, > + { 0x214c, 0x0003 }, > + { 0x4064, 0x0003 }, > + }, > + }, { > + 226666000, { > + { 0x0040, 0x0003 }, > + { 0x214c, 0x0003 }, > + { 0x4064, 0x0003 }, > + }, > + }, { > + 272000000, { > + { 0x0040, 0x0003 }, > + { 0x214c, 0x0003 }, > + { 0x5a64, 0x0003 }, > + }, > + }, { > + 340000000, { > + { 0x0040, 0x0003 }, > + { 0x3b4c, 0x0003 }, > + { 0x5a64, 0x0003 }, > + }, > + }, { > + 600000000, { 594000000 is the proper clock rate. > + { 0x1a40, 0x0003 }, > + { 0x3b4c, 0x0003 }, > + { 0x5a64, 0x0003 }, > + }, > + }, { > + ~0UL, { > + { 0x0000, 0x0000 }, > + { 0x0000, 0x0000 }, > + { 0x0000, 0x0000 }, > + }, > + } > +}; > + > +static const struct dw_hdmi_curr_ctrl sun50i_h6_cur_ctr[] = { The BSP sometimes uses different settings depending on the pixel repetition. Any ideas about this? I assume it's because the TMDS clock changes as a result of pixel repetition, as is the same with different bit depths. > + /* pixelclk bpp8 bpp10 bpp12 */ > + { 25175000, { 0x0000, 0x0000, 0x0000 }, }, > + { 27000000, { 0x0012, 0x0000, 0x0000 }, }, > + { 59400000, { 0x0008, 0x0008, 0x0008 }, }, > + { 72000000, { 0x0008, 0x0008, 0x001b }, }, > + { 74250000, { 0x0013, 0x0013, 0x0013 }, }, > + { 90000000, { 0x0008, 0x001a, 0x001b }, }, > + { 118800000, { 0x001b, 0x001a, 0x001b }, }, > + { 144000000, { 0x001b, 0x001a, 0x0034 }, }, > + { 180000000, { 0x001b, 0x0033, 0x0034 }, }, > + { 216000000, { 0x0036, 0x0033, 0x0034 }, }, > + { 237600000, { 0x0036, 0x0033, 0x001b }, }, > + { 288000000, { 0x0036, 0x001b, 0x001b }, }, > + { 297000000, { 0x0019, 0x001b, 0x0019 }, }, > + { 330000000, { 0x0036, 0x001b, 0x001b }, }, > + { 600000000, { 0x003f, 0x001b, 0x001b }, }, > + { ~0UL, { 0x0000, 0x0000, 0x0000 }, } > +}; > + > +static const struct dw_hdmi_phy_config sun50i_h6_phy_config[] = { > + /*pixelclk symbol term vlev*/ I see a slightly different setting for 27000000, and this frequency below 74250000 only, and even different for all three bit depths (not listed here). So: { 25175000, 0x8009, 0x0004, 0x0232 }, { 27000000, 0x8009, 0x0007, 0x02b0 }, And, > + { 74250000, 0x8009, 0x0004, 0x0232}, > + { 148500000, 0x8029, 0x0004, 0x0273}, These two don't match what I see in the BSP. BTW, which table did you use? phy301 or phy303? The code seems to indicate that model 301 is the one. > + { 600000000, 0x8039, 0x0004, 0x014a}, 594000000 is the proper pixel clock you're after. > + { ~0UL, 0x0000, 0x0000, 0x0000} > +}; > + > static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, > struct sun8i_hdmi_phy *phy, > unsigned int clk_rate) > @@ -290,6 +406,16 @@ static void sun8i_hdmi_phy_unlock(struct sun8i_hdmi_phy *phy) > SUN8I_HDMI_PHY_UNSCRAMBLE_MAGIC); > } > > +static void sun50i_hdmi_phy_init_h6(struct sun8i_hdmi_phy *phy) > +{ > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN); > + > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > + 0xffff0000, 0x80c00000); > +} > + > static void sun8i_hdmi_phy_init_a83t(struct sun8i_hdmi_phy *phy) > { > sun8i_hdmi_phy_unlock(phy); > @@ -423,6 +549,13 @@ static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { > .phy_config = &sun8i_hdmi_phy_config_h3, > }; > > +static const struct sun8i_hdmi_phy_variant sun50i_h6_hdmi_phy = { > + .cur_ctr = sun50i_h6_cur_ctr, > + .mpll_cfg = sun50i_h6_mpll_cfg, > + .phy_cfg = sun50i_h6_phy_config, > + .phy_init = &sun50i_hdmi_phy_init_h6, > +}; > + > static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { > .is_custom_phy = true, > .phy_init = &sun8i_hdmi_phy_init_a83t, > @@ -443,6 +576,10 @@ static const struct of_device_id sun8i_hdmi_phy_of_table[] = { > .compatible = "allwinner,sun50i-a64-hdmi-phy", > .data = &sun50i_a64_hdmi_phy, > }, > + { > + .compatible = "allwinner,sun50i-h6-hdmi-phy", > + .data = &sun50i_h6_hdmi_phy, > + }, Version sort please. Regards ChenYu > { > .compatible = "allwinner,sun8i-a83t-hdmi-phy", > .data = &sun8i_a83t_hdmi_phy, > -- > 2.18.0 > On Sun, Sep 2, 2018 at 3:28 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > H6 has Synopsys DWC HDMI 2.0 TX PHY. > > mpll settings were calculated from specifications of similar Synopsys > HDMI PHY found in i.MX6. Other PHY settings were derived from BSP PHY > driver code. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 137 +++++++++++++++++++++++++ > 1 file changed, 137 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > index ee2bf61cd4d2..2f5499bd35ec 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > @@ -14,6 +14,122 @@ > */ > #define I2C_ADDR 0x69 > > +static const struct dw_hdmi_mpll_config sun50i_h6_mpll_cfg[] = { > + { > + 30666000, { > + { 0x00b3, 0x0000 }, > + { 0x2153, 0x0000 }, > + { 0x40f3, 0x0000 }, > + }, > + }, { > + 36800000, { > + { 0x00b3, 0x0000 }, > + { 0x2153, 0x0000 }, > + { 0x40a2, 0x0001 }, > + }, > + }, { > + 46000000, { > + { 0x00b3, 0x0000 }, > + { 0x2142, 0x0001 }, > + { 0x40a2, 0x0001 }, > + }, > + }, { > + 61333000, { > + { 0x0072, 0x0001 }, > + { 0x2142, 0x0001 }, > + { 0x40a2, 0x0001 }, > + }, > + }, { > + 73600000, { > + { 0x0072, 0x0001 }, > + { 0x2142, 0x0001 }, > + { 0x4061, 0x0002 }, > + }, > + }, { > + 92000000, { > + { 0x0072, 0x0001 }, > + { 0x2145, 0x0002 }, > + { 0x4061, 0x0002 }, > + }, > + }, { > + 122666000, { > + { 0x0051, 0x0002 }, > + { 0x2145, 0x0002 }, > + { 0x4061, 0x0002 }, > + }, > + }, { > + 147200000, { > + { 0x0051, 0x0002 }, > + { 0x2145, 0x0002 }, > + { 0x4064, 0x0003 }, > + }, > + }, { > + 184000000, { > + { 0x0051, 0x0002 }, > + { 0x214c, 0x0003 }, > + { 0x4064, 0x0003 }, > + }, > + }, { > + 226666000, { > + { 0x0040, 0x0003 }, > + { 0x214c, 0x0003 }, > + { 0x4064, 0x0003 }, > + }, > + }, { > + 272000000, { > + { 0x0040, 0x0003 }, > + { 0x214c, 0x0003 }, > + { 0x5a64, 0x0003 }, > + }, > + }, { > + 340000000, { > + { 0x0040, 0x0003 }, > + { 0x3b4c, 0x0003 }, > + { 0x5a64, 0x0003 }, > + }, > + }, { > + 600000000, { > + { 0x1a40, 0x0003 }, > + { 0x3b4c, 0x0003 }, > + { 0x5a64, 0x0003 }, > + }, > + }, { > + ~0UL, { > + { 0x0000, 0x0000 }, > + { 0x0000, 0x0000 }, > + { 0x0000, 0x0000 }, > + }, > + } > +}; > + > +static const struct dw_hdmi_curr_ctrl sun50i_h6_cur_ctr[] = { > + /* pixelclk bpp8 bpp10 bpp12 */ > + { 25175000, { 0x0000, 0x0000, 0x0000 }, }, > + { 27000000, { 0x0012, 0x0000, 0x0000 }, }, > + { 59400000, { 0x0008, 0x0008, 0x0008 }, }, > + { 72000000, { 0x0008, 0x0008, 0x001b }, }, > + { 74250000, { 0x0013, 0x0013, 0x0013 }, }, > + { 90000000, { 0x0008, 0x001a, 0x001b }, }, > + { 118800000, { 0x001b, 0x001a, 0x001b }, }, > + { 144000000, { 0x001b, 0x001a, 0x0034 }, }, > + { 180000000, { 0x001b, 0x0033, 0x0034 }, }, > + { 216000000, { 0x0036, 0x0033, 0x0034 }, }, > + { 237600000, { 0x0036, 0x0033, 0x001b }, }, > + { 288000000, { 0x0036, 0x001b, 0x001b }, }, > + { 297000000, { 0x0019, 0x001b, 0x0019 }, }, > + { 330000000, { 0x0036, 0x001b, 0x001b }, }, > + { 600000000, { 0x003f, 0x001b, 0x001b }, }, > + { ~0UL, { 0x0000, 0x0000, 0x0000 }, } > +}; > + > +static const struct dw_hdmi_phy_config sun50i_h6_phy_config[] = { > + /*pixelclk symbol term vlev*/ > + { 74250000, 0x8009, 0x0004, 0x0232}, > + { 148500000, 0x8029, 0x0004, 0x0273}, > + { 600000000, 0x8039, 0x0004, 0x014a}, > + { ~0UL, 0x0000, 0x0000, 0x0000} > +}; > + > static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, > struct sun8i_hdmi_phy *phy, > unsigned int clk_rate) > @@ -290,6 +406,16 @@ static void sun8i_hdmi_phy_unlock(struct sun8i_hdmi_phy *phy) > SUN8I_HDMI_PHY_UNSCRAMBLE_MAGIC); > } > > +static void sun50i_hdmi_phy_init_h6(struct sun8i_hdmi_phy *phy) > +{ > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN); > + > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > + 0xffff0000, 0x80c00000); > +} > + > static void sun8i_hdmi_phy_init_a83t(struct sun8i_hdmi_phy *phy) > { > sun8i_hdmi_phy_unlock(phy); > @@ -423,6 +549,13 @@ static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { > .phy_config = &sun8i_hdmi_phy_config_h3, > }; > > +static const struct sun8i_hdmi_phy_variant sun50i_h6_hdmi_phy = { > + .cur_ctr = sun50i_h6_cur_ctr, > + .mpll_cfg = sun50i_h6_mpll_cfg, > + .phy_cfg = sun50i_h6_phy_config, > + .phy_init = &sun50i_hdmi_phy_init_h6, > +}; > + > static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { > .is_custom_phy = true, > .phy_init = &sun8i_hdmi_phy_init_a83t, > @@ -443,6 +576,10 @@ static const struct of_device_id sun8i_hdmi_phy_of_table[] = { > .compatible = "allwinner,sun50i-a64-hdmi-phy", > .data = &sun50i_a64_hdmi_phy, > }, > + { > + .compatible = "allwinner,sun50i-h6-hdmi-phy", > + .data = &sun50i_h6_hdmi_phy, > + }, > { > .compatible = "allwinner,sun8i-a83t-hdmi-phy", > .data = &sun8i_a83t_hdmi_phy, > -- > 2.18.0 >
Dne sobota, 22. september 2018 ob 17:55:35 CEST je Chen-Yu Tsai napisal(a): > On Sun, Sep 2, 2018 at 3:28 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > H6 has Synopsys DWC HDMI 2.0 TX PHY. > > > > mpll settings were calculated from specifications of similar Synopsys > > HDMI PHY found in i.MX6. Other PHY settings were derived from BSP PHY > > driver code. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 137 +++++++++++++++++++++++++ > > 1 file changed, 137 insertions(+) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index ee2bf61cd4d2..2f5499bd35ec > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > @@ -14,6 +14,122 @@ > > > > */ > > > > #define I2C_ADDR 0x69 > > > > +static const struct dw_hdmi_mpll_config sun50i_h6_mpll_cfg[] = { > > How did you choose the pixel clock points for this table? The values > sort of match up with the BSP. I used this script: https://chromium.googlesource.com/chromiumos/platform/drm-tests/+/master/ rk3288_hdmitables.py I forgot already how I came to conclusion that there are identical register settings. After all, imx6 manual describes DWC HDMI PHY and it seems that settings are identical for DWC HDMI PHY contained in imx6, rk3288 and h6. Check out that script, it has some explanation how values are calculated. Part of the reason why I didn't use original settings is that it is pain to convert BSP PHY settings table to this form. Not all frequencies have settings for 8, 10 and 12 bit depth. > > > + { > > + 30666000, { > > + { 0x00b3, 0x0000 }, > > + { 0x2153, 0x0000 }, > > + { 0x40f3, 0x0000 }, > > + }, > > + }, { > > + 36800000, { > > + { 0x00b3, 0x0000 }, > > + { 0x2153, 0x0000 }, > > + { 0x40a2, 0x0001 }, > > + }, > > + }, { > > + 46000000, { > > + { 0x00b3, 0x0000 }, > > + { 0x2142, 0x0001 }, > > + { 0x40a2, 0x0001 }, > > + }, > > + }, { > > + 61333000, { > > + { 0x0072, 0x0001 }, > > + { 0x2142, 0x0001 }, > > + { 0x40a2, 0x0001 }, > > + }, > > + }, { > > + 73600000, { > > + { 0x0072, 0x0001 }, > > + { 0x2142, 0x0001 }, > > + { 0x4061, 0x0002 }, > > + }, > > + }, { > > + 92000000, { > > + { 0x0072, 0x0001 }, > > + { 0x2145, 0x0002 }, > > + { 0x4061, 0x0002 }, > > + }, > > + }, { > > + 122666000, { > > + { 0x0051, 0x0002 }, > > + { 0x2145, 0x0002 }, > > + { 0x4061, 0x0002 }, > > + }, > > + }, { > > + 147200000, { > > + { 0x0051, 0x0002 }, > > + { 0x2145, 0x0002 }, > > + { 0x4064, 0x0003 }, > > + }, > > + }, { > > + 184000000, { > > + { 0x0051, 0x0002 }, > > + { 0x214c, 0x0003 }, > > + { 0x4064, 0x0003 }, > > + }, > > + }, { > > + 226666000, { > > + { 0x0040, 0x0003 }, > > + { 0x214c, 0x0003 }, > > + { 0x4064, 0x0003 }, > > + }, > > + }, { > > + 272000000, { > > + { 0x0040, 0x0003 }, > > + { 0x214c, 0x0003 }, > > + { 0x5a64, 0x0003 }, > > + }, > > + }, { > > + 340000000, { > > + { 0x0040, 0x0003 }, > > + { 0x3b4c, 0x0003 }, > > + { 0x5a64, 0x0003 }, > > + }, > > + }, { > > + 600000000, { > > 594000000 is the proper clock rate. > > > + { 0x1a40, 0x0003 }, > > + { 0x3b4c, 0x0003 }, > > + { 0x5a64, 0x0003 }, > > + }, > > + }, { > > + ~0UL, { > > + { 0x0000, 0x0000 }, > > + { 0x0000, 0x0000 }, > > + { 0x0000, 0x0000 }, > > + }, > > + } > > +}; > > + > > +static const struct dw_hdmi_curr_ctrl sun50i_h6_cur_ctr[] = { > > The BSP sometimes uses different settings depending on the pixel repetition. > Any ideas about this? I assume it's because the TMDS clock changes as a > result of pixel repetition, as is the same with different bit depths. No, no idea. As I said before, I went easy way out using script mentioned before. > > + /* pixelclk bpp8 bpp10 bpp12 */ > > + { 25175000, { 0x0000, 0x0000, 0x0000 }, }, > > + { 27000000, { 0x0012, 0x0000, 0x0000 }, }, > > + { 59400000, { 0x0008, 0x0008, 0x0008 }, }, > > + { 72000000, { 0x0008, 0x0008, 0x001b }, }, > > + { 74250000, { 0x0013, 0x0013, 0x0013 }, }, > > + { 90000000, { 0x0008, 0x001a, 0x001b }, }, > > + { 118800000, { 0x001b, 0x001a, 0x001b }, }, > > + { 144000000, { 0x001b, 0x001a, 0x0034 }, }, > > + { 180000000, { 0x001b, 0x0033, 0x0034 }, }, > > + { 216000000, { 0x0036, 0x0033, 0x0034 }, }, > > + { 237600000, { 0x0036, 0x0033, 0x001b }, }, > > + { 288000000, { 0x0036, 0x001b, 0x001b }, }, > > + { 297000000, { 0x0019, 0x001b, 0x0019 }, }, > > + { 330000000, { 0x0036, 0x001b, 0x001b }, }, > > + { 600000000, { 0x003f, 0x001b, 0x001b }, }, > > + { ~0UL, { 0x0000, 0x0000, 0x0000 }, } > > +}; > > + > > +static const struct dw_hdmi_phy_config sun50i_h6_phy_config[] = { > > + /*pixelclk symbol term vlev*/ > > I see a slightly different setting for 27000000, and this frequency > below 74250000 > only, and even different for all three bit depths (not listed here). So: > > { 25175000, 0x8009, 0x0004, 0x0232 }, > { 27000000, 0x8009, 0x0007, 0x02b0 }, > > And, > > > + { 74250000, 0x8009, 0x0004, 0x0232}, > > + { 148500000, 0x8029, 0x0004, 0x0273}, > > These two don't match what I see in the BSP. BTW, which table did you use? > phy301 or phy303? The code seems to indicate that model 301 is the one. > yes, BSP uses 301. Best regards, Jernej > > + { 600000000, 0x8039, 0x0004, 0x014a}, > > 594000000 is the proper pixel clock you're after. > > > + { ~0UL, 0x0000, 0x0000, 0x0000} > > +}; > > + > > > > static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, > > > > struct sun8i_hdmi_phy *phy, > > unsigned int clk_rate) > > > > @@ -290,6 +406,16 @@ static void sun8i_hdmi_phy_unlock(struct > > sun8i_hdmi_phy *phy)> > > SUN8I_HDMI_PHY_UNSCRAMBLE_MAGIC); > > > > } > > > > +static void sun50i_hdmi_phy_init_h6(struct sun8i_hdmi_phy *phy) > > +{ > > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, > > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN); > > + > > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > > + 0xffff0000, 0x80c00000); > > +} > > + > > > > static void sun8i_hdmi_phy_init_a83t(struct sun8i_hdmi_phy *phy) > > { > > > > sun8i_hdmi_phy_unlock(phy); > > > > @@ -423,6 +549,13 @@ static const struct sun8i_hdmi_phy_variant > > sun50i_a64_hdmi_phy = {> > > .phy_config = &sun8i_hdmi_phy_config_h3, > > > > }; > > > > +static const struct sun8i_hdmi_phy_variant sun50i_h6_hdmi_phy = { > > + .cur_ctr = sun50i_h6_cur_ctr, > > + .mpll_cfg = sun50i_h6_mpll_cfg, > > + .phy_cfg = sun50i_h6_phy_config, > > + .phy_init = &sun50i_hdmi_phy_init_h6, > > +}; > > + > > > > static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { > > > > .is_custom_phy = true, > > .phy_init = &sun8i_hdmi_phy_init_a83t, > > > > @@ -443,6 +576,10 @@ static const struct of_device_id > > sun8i_hdmi_phy_of_table[] = {> > > .compatible = "allwinner,sun50i-a64-hdmi-phy", > > .data = &sun50i_a64_hdmi_phy, > > > > }, > > > > + { > > + .compatible = "allwinner,sun50i-h6-hdmi-phy", > > + .data = &sun50i_h6_hdmi_phy, > > + }, > > Version sort please. > > Regards > ChenYu > > > { > > > > .compatible = "allwinner,sun8i-a83t-hdmi-phy", > > .data = &sun8i_a83t_hdmi_phy, > > > > -- > > 2.18.0 > > On Sun, Sep 2, 2018 at 3:28 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > H6 has Synopsys DWC HDMI 2.0 TX PHY. > > > > mpll settings were calculated from specifications of similar Synopsys > > HDMI PHY found in i.MX6. Other PHY settings were derived from BSP PHY > > driver code. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 137 +++++++++++++++++++++++++ > > 1 file changed, 137 insertions(+) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index ee2bf61cd4d2..2f5499bd35ec > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > @@ -14,6 +14,122 @@ > > > > */ > > > > #define I2C_ADDR 0x69 > > > > +static const struct dw_hdmi_mpll_config sun50i_h6_mpll_cfg[] = { > > + { > > + 30666000, { > > + { 0x00b3, 0x0000 }, > > + { 0x2153, 0x0000 }, > > + { 0x40f3, 0x0000 }, > > + }, > > + }, { > > + 36800000, { > > + { 0x00b3, 0x0000 }, > > + { 0x2153, 0x0000 }, > > + { 0x40a2, 0x0001 }, > > + }, > > + }, { > > + 46000000, { > > + { 0x00b3, 0x0000 }, > > + { 0x2142, 0x0001 }, > > + { 0x40a2, 0x0001 }, > > + }, > > + }, { > > + 61333000, { > > + { 0x0072, 0x0001 }, > > + { 0x2142, 0x0001 }, > > + { 0x40a2, 0x0001 }, > > + }, > > + }, { > > + 73600000, { > > + { 0x0072, 0x0001 }, > > + { 0x2142, 0x0001 }, > > + { 0x4061, 0x0002 }, > > + }, > > + }, { > > + 92000000, { > > + { 0x0072, 0x0001 }, > > + { 0x2145, 0x0002 }, > > + { 0x4061, 0x0002 }, > > + }, > > + }, { > > + 122666000, { > > + { 0x0051, 0x0002 }, > > + { 0x2145, 0x0002 }, > > + { 0x4061, 0x0002 }, > > + }, > > + }, { > > + 147200000, { > > + { 0x0051, 0x0002 }, > > + { 0x2145, 0x0002 }, > > + { 0x4064, 0x0003 }, > > + }, > > + }, { > > + 184000000, { > > + { 0x0051, 0x0002 }, > > + { 0x214c, 0x0003 }, > > + { 0x4064, 0x0003 }, > > + }, > > + }, { > > + 226666000, { > > + { 0x0040, 0x0003 }, > > + { 0x214c, 0x0003 }, > > + { 0x4064, 0x0003 }, > > + }, > > + }, { > > + 272000000, { > > + { 0x0040, 0x0003 }, > > + { 0x214c, 0x0003 }, > > + { 0x5a64, 0x0003 }, > > + }, > > + }, { > > + 340000000, { > > + { 0x0040, 0x0003 }, > > + { 0x3b4c, 0x0003 }, > > + { 0x5a64, 0x0003 }, > > + }, > > + }, { > > + 600000000, { > > + { 0x1a40, 0x0003 }, > > + { 0x3b4c, 0x0003 }, > > + { 0x5a64, 0x0003 }, > > + }, > > + }, { > > + ~0UL, { > > + { 0x0000, 0x0000 }, > > + { 0x0000, 0x0000 }, > > + { 0x0000, 0x0000 }, > > + }, > > + } > > +}; > > + > > +static const struct dw_hdmi_curr_ctrl sun50i_h6_cur_ctr[] = { > > + /* pixelclk bpp8 bpp10 bpp12 */ > > + { 25175000, { 0x0000, 0x0000, 0x0000 }, }, > > + { 27000000, { 0x0012, 0x0000, 0x0000 }, }, > > + { 59400000, { 0x0008, 0x0008, 0x0008 }, }, > > + { 72000000, { 0x0008, 0x0008, 0x001b }, }, > > + { 74250000, { 0x0013, 0x0013, 0x0013 }, }, > > + { 90000000, { 0x0008, 0x001a, 0x001b }, }, > > + { 118800000, { 0x001b, 0x001a, 0x001b }, }, > > + { 144000000, { 0x001b, 0x001a, 0x0034 }, }, > > + { 180000000, { 0x001b, 0x0033, 0x0034 }, }, > > + { 216000000, { 0x0036, 0x0033, 0x0034 }, }, > > + { 237600000, { 0x0036, 0x0033, 0x001b }, }, > > + { 288000000, { 0x0036, 0x001b, 0x001b }, }, > > + { 297000000, { 0x0019, 0x001b, 0x0019 }, }, > > + { 330000000, { 0x0036, 0x001b, 0x001b }, }, > > + { 600000000, { 0x003f, 0x001b, 0x001b }, }, > > + { ~0UL, { 0x0000, 0x0000, 0x0000 }, } > > +}; > > + > > +static const struct dw_hdmi_phy_config sun50i_h6_phy_config[] = { > > + /*pixelclk symbol term vlev*/ > > + { 74250000, 0x8009, 0x0004, 0x0232}, > > + { 148500000, 0x8029, 0x0004, 0x0273}, > > + { 600000000, 0x8039, 0x0004, 0x014a}, > > + { ~0UL, 0x0000, 0x0000, 0x0000} > > +}; > > + > > > > static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi, > > > > struct sun8i_hdmi_phy *phy, > > unsigned int clk_rate) > > > > @@ -290,6 +406,16 @@ static void sun8i_hdmi_phy_unlock(struct > > sun8i_hdmi_phy *phy)> > > SUN8I_HDMI_PHY_UNSCRAMBLE_MAGIC); > > > > } > > > > +static void sun50i_hdmi_phy_init_h6(struct sun8i_hdmi_phy *phy) > > +{ > > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN, > > + SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN); > > + > > + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG, > > + 0xffff0000, 0x80c00000); > > +} > > + > > > > static void sun8i_hdmi_phy_init_a83t(struct sun8i_hdmi_phy *phy) > > { > > > > sun8i_hdmi_phy_unlock(phy); > > > > @@ -423,6 +549,13 @@ static const struct sun8i_hdmi_phy_variant > > sun50i_a64_hdmi_phy = {> > > .phy_config = &sun8i_hdmi_phy_config_h3, > > > > }; > > > > +static const struct sun8i_hdmi_phy_variant sun50i_h6_hdmi_phy = { > > + .cur_ctr = sun50i_h6_cur_ctr, > > + .mpll_cfg = sun50i_h6_mpll_cfg, > > + .phy_cfg = sun50i_h6_phy_config, > > + .phy_init = &sun50i_hdmi_phy_init_h6, > > +}; > > + > > > > static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { > > > > .is_custom_phy = true, > > .phy_init = &sun8i_hdmi_phy_init_a83t, > > > > @@ -443,6 +576,10 @@ static const struct of_device_id > > sun8i_hdmi_phy_of_table[] = {> > > .compatible = "allwinner,sun50i-a64-hdmi-phy", > > .data = &sun50i_a64_hdmi_phy, > > > > }, > > > > + { > > + .compatible = "allwinner,sun50i-h6-hdmi-phy", > > + .data = &sun50i_h6_hdmi_phy, > > + }, > > > > { > > > > .compatible = "allwinner,sun8i-a83t-hdmi-phy", > > .data = &sun8i_a83t_hdmi_phy, > > > > -- > > 2.18.0
Dne sobota, 22. september 2018 ob 15:47:03 CEST je Chen-Yu Tsai napisal(a): > On Sat, Sep 22, 2018 at 9:23 PM Chen-Yu Tsai <wens@csie.org> wrote: > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > > Mixer 0 has 1 VI and 3 UI planes, scaler on all planes and can output > > > 4K image @60Hz. It also support 10 bit colors. > > > > AFAICT 10 bit color support is not implemented? Please mention this. ok. > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index a9218abf0935..54eca2dd4b33 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > @@ -540,6 +540,15 @@ static int sun8i_mixer_remove(struct > > > platform_device *pdev)> > > > > return 0; > > > > > > } > > > > > > +static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = { > > > > Please sort the per-compatible structures according to "version sort" > > rules.> > > > + .ccsc = 0, > > > + .is_de3 = true, > > > + .mod_rate = 600000000, > > > + .scaler_mask = 0xf, > > > + .ui_num = 3, > > > + .vi_num = 1, > > > +}; > > > + > > > > > > static const struct sun8i_mixer_cfg sun8i_a83t_mixer0_cfg = { > > > > > > .ccsc = 0, > > > .scaler_mask = 0xf, > > > > > > @@ -587,6 +596,10 @@ static const struct sun8i_mixer_cfg > > > sun8i_v3s_mixer_cfg = {> > > > > }; > > > > > > static const struct of_device_id sun8i_mixer_of_table[] = { > > > > > > + { > > > + .compatible = "allwinner,sun50i-h6-de3-mixer-0", > > > + .data = &sun50i_h6_mixer0_cfg, > > > + }, > > > > Same here. > > > > ChenYu > > BTW, DE 3.0 includes a register in DE TOP called "DE IP configure register", > which gives the number of IP blocks per class, per mixer. If we retrieve > the configuration from this register, then we shouldn't need to > differentiate between mixer-0 and mixer-1 with compatible strings. > > What do you think? IIRC, not all setting were correct when read from registers, but I would need to check again. I'm also not sure if register holds all possible settings, so it is safer to have separate list. We would also have to devise mechanism to get this data from DE2/3 CCU driver (it occupies the same memory space). Perhaps the strongest argument is that some SoCs with DE3 have HW bug in mixer1 block, including that in H6. In order to work, mod clock has to be enabled for mixer0 and mixer1 at the same time. I would associate that quirk with mixer1 compatible. Best regards, Jernej
Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a): > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Display Engine 3 is an upgrade of DE2 with new features like support for > > 10 bit color formats and support for AFBC. > > > > Most of DE2 code works with DE3, except some small details. > > > > Add support for it. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_csc.c | 96 +++++++++++++++++++++++-- > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 17 ++++- > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 21 +++++- > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 8 ++- > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.h | 1 + > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 8 +++ > > drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 2 + > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++- > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++ > > 9 files changed, 197 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c > > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c > > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = { > > > > 0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A, > > > > }; > > > > +/* > > + * DE3 has a bit different CSC units. Factors are in two's complement > > format. + * First three have 17 bits for fractinal part and last two 2 > > bits. First + * three values in each line are multiplication factor, 4th > > is difference, + * which is subtracted from the input value before the > > multiplication and + * last value is constant, which is added at the end. > > + * > > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0 > > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1 > > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2 > > + * > > + * Please note that above formula is true only for Blender CSC. Other DE3 > > CSC + * units takes only positive value for difference. From what can be > > deducted + * from BSP driver code, those units probably automatically > > assume that + * difference has to be subtracted. > > + */ > > +static const u32 yuv2rgb_de3[] = { > > + 0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000, > > + 0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000, > > + 0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000, > > +}; > > + > > +static const u32 yvu2rgb_de3[] = { > > + 0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000, > > + 0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000, > > + 0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000, > > +}; > > + > > > > static void sun8i_csc_set_coefficients(struct regmap *map, u32 base, > > > > enum sun8i_csc_mode mode) > > > > { > > > > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap > > *map, u32 base,> > > } > > > > } > > > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int > > layer, > > + enum sun8i_csc_mode mode) > > +{ > > + const u32 *table; > > + int i, j; > > + > > + switch (mode) { > > + case SUN8I_CSC_MODE_YUV2RGB: > > + table = yuv2rgb_de3; > > + break; > > + case SUN8I_CSC_MODE_YVU2RGB: > > + table = yvu2rgb_de3; > > + break; > > + default: > > + DRM_WARN("Wrong CSC mode specified.\n"); > > + return; > > + } > > + > > + for (i = 0; i < 3; i++) { > > + for (j = 0; j < 3; j++) > > + regmap_write(map, > > + > > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, + > > layer, i, j), + > > table[i * 5 + j]); > > Given that the first three values occupy contiguous addresses, > you can use regmap_bulk_write() here. > > > + regmap_write(map, > > + SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE, > > + layer, i), > > + SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 + > > 3], + table[i > > * 5 + 4])); > Nit: Using a two-dimension array might make it easier to read. > > > + } > > +} > > + > > > > static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable) > > { > > > > u32 val; > > > > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32 > > base, bool enable)> > > regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN, > > val); > > > > } > > > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool > > enable) +{ > > + u32 val, mask; > > + > > + mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer); > > + > > + if (enable) > > + val = mask; > > + else > > + val = 0; > > + > > + regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE), > > + mask, val); > > +} > > + > > > > void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int > > layer, > > > > enum sun8i_csc_mode mode) > > > > { > > > > - u32 base; > > + if (!mixer->cfg->is_de3) { > > + u32 base; > > You could rewrite this as > > if (mixer->cfg->is_de3) { > sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > layer, mode); > return; > } > > That way you don't need to change the indentation on the existing lines. > I suppose this is more of a personal preference. The downside is the control > flow is slightly more complicated. > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > - sun8i_csc_set_coefficients(mixer->engine.regs, base, mode); > > + sun8i_csc_set_coefficients(mixer->engine.regs, base, > > mode); > > + } else { > > + sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > > + layer, mode); > > + } > > > > } > > > > void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool > > enable) { > > > > - u32 base; > > + if (!mixer->cfg->is_de3) { > > + u32 base; > > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > - sun8i_csc_enable(mixer->engine.regs, base, enable); > > + sun8i_csc_enable(mixer->engine.regs, base, enable); > > + } else { > > + sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable); > > + } > > > > } > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev, > > struct device *master,> > > base = sun8i_blender_base(mixer); > > > > /* Reset the registers */ > > > > - for (i = 0x0; i < 0x20000; i += 4) > > - regmap_write(mixer->engine.regs, i, 0); > > + if (mixer->cfg->is_de3) { > > + for (i = 0x0; i < 0x3000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0x20000; i < 0x40000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0x70000; i < 0x88000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0xa0000; i < 0xb0000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0xd0000; i < 0xe0000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > Could you use the macros *_BASE and *_SIZE here? That would make > it more obviously what parts you're clearing. Ok, but where should I define them? In sun8i-mixer.h? > > The last offset, 0xd0000, isn't even defined in the DE3 spec. It is, check chapter 2.3.2, table 2-3. > > > + } else { > > + for (i = 0x0; i < 0x20000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + } > > > > /* Enable the mixer */ > > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL, > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > > @@ -33,6 +33,10 @@ > > > > #define DE2_CH_BASE 0x2000 > > #define DE2_CH_SIZE 0x1000 > > > > +#define DE3_BLD_BASE 0x0800 > > +#define DE3_CH_BASE 0x1000 > > +#define DE3_CH_SIZE 0x0800 > > + > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > > #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * > > (x)) > > #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * > > (x)) > > > > @@ -47,6 +51,11 @@ > > > > #define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * > > (x)) > > #define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + 0x04 * > > (x)) > > #define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + 0xfc) > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL(base) ((base) + 0x100) > > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \ > > + ((base) + 0x110 + (layer) * 0x30 + (x) * 0x10 + 4 * (y)) > > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \ > > + ((base) + 0x110 + (layer) * 0x30 + (i) * 0x10 + 0x0c) > > Should these have a DE3 prefix? Ok for all prefix comments. > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > > > > @@ -61,6 +70,9 @@ > > > > #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED BIT(1) > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch) BIT(ch) > > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c) (((d) << 16) | ((c) & > > 0xffff)) + > > Same here. > > > #define SUN8I_MIXER_FBFMT_ARGB8888 0 > > #define SUN8I_MIXER_FBFMT_ABGR8888 1 > > #define SUN8I_MIXER_FBFMT_RGBA8888 2 > > [...] > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > @@ -30,6 +30,8 @@ > > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE BIT(15) > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET 8 > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK GENMASK(12, 8) > > > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK GENMASK(31, 24) > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x) ((x) << > > 24) > > DE3 prefix? > > > struct sun8i_mixer; > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index > > 8697afc36023..22e1ed5cd7a4 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = { > > > > static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int > > channel) { > > > > - return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > + if (mixer->cfg->is_de3) > > + return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel; > > + else > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > > } > > > > static int sun8i_vi_scaler_coef_index(unsigned int step) > > > > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, > > int layer,> > > cvphase = vphase; > > > > } > > > > + if (mixer->cfg->is_de3) { > > + u32 val; > > + > > + if (format->hsub == 1 && format->vsub == 1) > > + val = SUN8I_SCALER_VSU_SCALE_MODE_UI; > > + else > > + val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL; > > + > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_SCALE_MODE(base), val); > > The remaining settings seem to be related to the edge detection scaling > method. Since you aren't supporting it, are they necessary? Didn't really tried without them IIRC. I will for next revision. Best regards, Jernej > > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_DIR_THR(base), > > + SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) | > > + SUN8I_SCALER_VSU_ZERO_DIR_THR(0) | > > + SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) | > > + SUN8I_SCALER_VSU_VERT_DIR_THR(1)); > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_EDGE_THR(base), > > + SUN8I_SCALER_VSU_EDGE_SHIFT(8) | > > + SUN8I_SCALER_VSU_EDGE_OFFSET(0)); > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0); > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_ANGLE_THR(base), > > + SUN8I_SCALER_VSU_ANGLE_SHIFT(2) | > > + SUN8I_SCALER_VSU_ANGLE_OFFSET(0)); > > + } > > + > > > > regmap_write(mixer->engine.regs, > > > > SUN8I_SCALER_VSU_OUTSIZE(base), outsize); > > > > regmap_write(mixer->engine.regs, > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index > > cd015405f66d..c322f5652481 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > @@ -15,6 +15,9 @@ > > > > #define DE2_VI_SCALER_BASE 0x20000 > > #define DE2_VI_SCALER_SIZE 0x20000 > > > > +#define DE3_VI_SCALER_BASE 0x20000 > > +#define DE3_VI_SCALER_SIZE 0x08000 > > + > > > > /* this two macros assumes 16 fractional bits which is standard in DRM */ > > #define SUN8I_VI_SCALER_SCALE_MIN 1 > > #define SUN8I_VI_SCALER_SCALE_MAX ((1UL << 20) - 1) > > > > @@ -25,6 +28,11 @@ > > > > #define SUN8I_VI_SCALER_SIZE(w, h) (((h) - 1) << 16 | ((w) - > > 1)) > > > > #define SUN8I_SCALER_VSU_CTRL(base) ((base) + 0x0) > > > > +#define SUN8I_SCALER_VSU_SCALE_MODE(base) ((base) + 0x10) > > +#define SUN8I_SCALER_VSU_DIR_THR(base) ((base) + 0x20) > > +#define SUN8I_SCALER_VSU_EDGE_THR(base) ((base) + 0x24) > > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base) ((base) + 0x28) > > +#define SUN8I_SCALER_VSU_ANGLE_THR(base) ((base) + 0x2c) > > DE3 prefix for the new ones please. > > > #define SUN8I_SCALER_VSU_OUTSIZE(base) ((base) + 0x40) > > #define SUN8I_SCALER_VSU_YINSIZE(base) ((base) + 0x80) > > #define SUN8I_SCALER_VSU_YHSTEP(base) ((base) + 0x88) > > > > @@ -46,6 +54,21 @@ > > > > #define SUN8I_SCALER_VSU_CTRL_EN BIT(0) > > #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY BIT(4) > > > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x) (((x) << 24) & 0xFF) > > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x) (((x) << 16) & 0xFF) > > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x) (((x) << 8) & 0xFF) > > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x) ((x) & 0xFF) > > + > > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI 0 > > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL 1 > > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE 2 > > + > > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x) (((x) << 16) & 0xF) > > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x) ((x) & 0xFF) > > + > > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x) (((x) << 16) & > > 0xF) > > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x) ((x) & 0xFF) > > + > > Same here. > > Regards > ChenYu > > > void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool > > enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int > > layer, > > > > u32 src_w, u32 src_h, u32 dst_w, u32 dst_h, > > > > -- > > 2.18.0
Dne sobota, 22. september 2018 ob 14:32:30 CEST je Chen-Yu Tsai napisal(a): > Hi, > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Most, if not all, registers found in DE2 still exists in DE3. However, > > units are on different base addresses. > > > > To prepare for addition of DE3 support, registers macros are reworked so > > they take base address as parameter. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > [rebased] > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > This patch mostly checks out. But see below. > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 406c42e752d7..020b0a097c84 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > > @@ -29,20 +29,24 @@ > > > > #define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE BIT(0) > > > > -#define SUN8I_MIXER_BLEND_PIPE_CTL 0x1000 > > -#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(x) (0x1004 + 0x10 * (x) + > > 0x0) > > -#define SUN8I_MIXER_BLEND_ATTR_INSIZE(x) (0x1004 + 0x10 * (x) + > > 0x4) > > -#define SUN8I_MIXER_BLEND_ATTR_COORD(x) (0x1004 + 0x10 * > > (x) + 0x8) -#define SUN8I_MIXER_BLEND_ROUTE 0x1080 > > -#define SUN8I_MIXER_BLEND_PREMULTIPLY 0x1084 > > -#define SUN8I_MIXER_BLEND_BKCOLOR 0x1088 > > -#define SUN8I_MIXER_BLEND_OUTSIZE 0x108c > > -#define SUN8I_MIXER_BLEND_MODE(x) (0x1090 + 0x04 * (x)) > > -#define SUN8I_MIXER_BLEND_CK_CTL 0x10b0 > > -#define SUN8I_MIXER_BLEND_CK_CFG 0x10b4 > > -#define SUN8I_MIXER_BLEND_CK_MAX(x) (0x10c0 + 0x04 * (x)) > > -#define SUN8I_MIXER_BLEND_CK_MIN(x) (0x10e0 + 0x04 * (x)) > > -#define SUN8I_MIXER_BLEND_OUTCTL 0x10fc > > +#define DE2_BLD_BASE 0x1000 > > +#define DE2_CH_BASE 0x2000 > > +#define DE2_CH_SIZE 0x1000 > > + > > +#define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > > +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * > > (x)) > > +#define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * > > (x)) > > +#define SUN8I_MIXER_BLEND_ATTR_COORD(base, x) ((base) + 0xC + 0x10 * > > (x)) > > Nit: Use lowercase for '0xC' to be consistent. > > > +#define SUN8I_MIXER_BLEND_ROUTE(base) ((base) + 0x80) > > +#define SUN8I_MIXER_BLEND_PREMULTIPLY(base) ((base) + 0x84) > > +#define SUN8I_MIXER_BLEND_BKCOLOR(base) ((base) + 0x88) > > +#define SUN8I_MIXER_BLEND_OUTSIZE(base) ((base) + 0x8c) > > +#define SUN8I_MIXER_BLEND_MODE(base, x) ((base) + 0x90 + > > 0x04 * (x)) +#define SUN8I_MIXER_BLEND_CK_CTL(base) ((base) + > > 0xb0) > > +#define SUN8I_MIXER_BLEND_CK_CFG(base) ((base) + 0xb4) > > +#define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * > > (x)) +#define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + > > 0x04 * (x)) +#define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + > > 0xfc) > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > > [...] > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > > b/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c index > > 6bb2aa164c8e..c68eab8a748f 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > > @@ -10,6 +10,7 @@ > > > > */ > > > > #include "sun8i_ui_scaler.h" > > > > +#include "sun8i_vi_scaler.h" > > > > static const u32 lan2coefftab16[240] = { > > > > 0x00004000, 0x00033ffe, 0x00063efc, 0x000a3bfb, > > > > @@ -88,6 +89,14 @@ static const u32 lan2coefftab16[240] = { > > > > 0x0b1c1603, 0x0d1c1502, 0x0e1d1401, 0x0f1d1301, > > > > }; > > > > +static inline u32 sun8i_ui_scaler_base(struct sun8i_mixer *mixer, int > > channel) > I recently saw a review comment stating one should not inline functions > unless they are defined in header files. Otherwise the decision should > be left up to the compiler. > > > +{ > > + int vi_num = mixer->cfg->vi_num; > > + > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * vi_num + > > + DE2_UI_SCALER_SIZE * (channel - vi_num); > > +} > > + > > > > static int sun8i_ui_scaler_coef_index(unsigned int step) > > { > > > > unsigned int scale, int_part, float_part; > > [...] > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index > > d3f1acb234b7..8697afc36023 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > @@ -833,6 +833,11 @@ static const u32 bicubic4coefftab32[480] = { > > > > 0x1012110d, 0x1012110d, 0x1013110c, 0x1013110c, > > > > }; > > > > +static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int > > channel) +{ > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > +} > > + > > This one as well. > > > static int sun8i_vi_scaler_coef_index(unsigned int step) > > { > > > > unsigned int scale, int_part, float_part; > > > > @@ -857,7 +862,7 @@ static int sun8i_vi_scaler_coef_index(unsigned int > > step)> > > } > > > > } > > > > -static void sun8i_vi_scaler_set_coeff(struct regmap *map, int layer, > > +static void sun8i_vi_scaler_set_coeff(struct regmap *map, u32 base, > > > > u32 hstep, u32 vstep, > > const struct drm_format_info > > *format) > > This is the only instance where a function's "layer" parameter was changed > to "base". It would be nice if it were consistent. Why not? Caller already have base address calculated, so it poses no overhead. Additionally, sun8i_vi_scaler_set_coeff() doesn't have struct sun8i_mixer *mixer parameter, which would allow calculating base address based on layer id. So, you can chose between existing variant or adding additional parameter for no real reason. Best regards, Jernej
On Mon, Sep 24, 2018 at 3:40 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne sobota, 22. september 2018 ob 15:47:03 CEST je Chen-Yu Tsai napisal(a): > > On Sat, Sep 22, 2018 at 9:23 PM Chen-Yu Tsai <wens@csie.org> wrote: > > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > wrote: > > > > Mixer 0 has 1 VI and 3 UI planes, scaler on all planes and can output > > > > 4K image @60Hz. It also support 10 bit colors. > > > > > > AFAICT 10 bit color support is not implemented? Please mention this. > > ok. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > --- > > > > > > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 13 +++++++++++++ > > > > 1 file changed, 13 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index a9218abf0935..54eca2dd4b33 > > > > 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > @@ -540,6 +540,15 @@ static int sun8i_mixer_remove(struct > > > > platform_device *pdev)> > > > > > return 0; > > > > > > > > } > > > > > > > > +static const struct sun8i_mixer_cfg sun50i_h6_mixer0_cfg = { > > > > > > Please sort the per-compatible structures according to "version sort" > > > rules.> > > > > + .ccsc = 0, > > > > + .is_de3 = true, > > > > + .mod_rate = 600000000, > > > > + .scaler_mask = 0xf, > > > > + .ui_num = 3, > > > > + .vi_num = 1, > > > > +}; > > > > + > > > > > > > > static const struct sun8i_mixer_cfg sun8i_a83t_mixer0_cfg = { > > > > > > > > .ccsc = 0, > > > > .scaler_mask = 0xf, > > > > > > > > @@ -587,6 +596,10 @@ static const struct sun8i_mixer_cfg > > > > sun8i_v3s_mixer_cfg = {> > > > > > }; > > > > > > > > static const struct of_device_id sun8i_mixer_of_table[] = { > > > > > > > > + { > > > > + .compatible = "allwinner,sun50i-h6-de3-mixer-0", > > > > + .data = &sun50i_h6_mixer0_cfg, > > > > + }, > > > > > > Same here. > > > > > > ChenYu > > > > BTW, DE 3.0 includes a register in DE TOP called "DE IP configure register", > > which gives the number of IP blocks per class, per mixer. If we retrieve > > the configuration from this register, then we shouldn't need to > > differentiate between mixer-0 and mixer-1 with compatible strings. > > > > What do you think? > > IIRC, not all setting were correct when read from registers, but I would need > to check again. I'm also not sure if register holds all possible settings, so > it is safer to have separate list. We would also have to devise mechanism to > get this data from DE2/3 CCU driver (it occupies the same memory space). > > Perhaps the strongest argument is that some SoCs with DE3 have HW bug in > mixer1 block, including that in H6. In order to work, mod clock has to be > enabled for mixer0 and mixer1 at the same time. I would associate that quirk > with mixer1 compatible. OK. That makes sense. So apart from the mentioning 10 bit support status in the commit log, Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Sep 24, 2018 at 4:02 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne sobota, 22. september 2018 ob 14:32:30 CEST je Chen-Yu Tsai napisal(a): > > Hi, > > > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > wrote: > > > Most, if not all, registers found in DE2 still exists in DE3. However, > > > units are on different base addresses. > > > > > > To prepare for addition of DE3 support, registers macros are reworked so > > > they take base address as parameter. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > [rebased] > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > This patch mostly checks out. But see below. > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 406c42e752d7..020b0a097c84 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > @@ -29,20 +29,24 @@ > > > > > > #define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE BIT(0) > > > > > > -#define SUN8I_MIXER_BLEND_PIPE_CTL 0x1000 > > > -#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(x) (0x1004 + 0x10 * (x) + > > > 0x0) > > > -#define SUN8I_MIXER_BLEND_ATTR_INSIZE(x) (0x1004 + 0x10 * (x) + > > > 0x4) > > > -#define SUN8I_MIXER_BLEND_ATTR_COORD(x) (0x1004 + 0x10 * > > > (x) + 0x8) -#define SUN8I_MIXER_BLEND_ROUTE 0x1080 > > > -#define SUN8I_MIXER_BLEND_PREMULTIPLY 0x1084 > > > -#define SUN8I_MIXER_BLEND_BKCOLOR 0x1088 > > > -#define SUN8I_MIXER_BLEND_OUTSIZE 0x108c > > > -#define SUN8I_MIXER_BLEND_MODE(x) (0x1090 + 0x04 * (x)) > > > -#define SUN8I_MIXER_BLEND_CK_CTL 0x10b0 > > > -#define SUN8I_MIXER_BLEND_CK_CFG 0x10b4 > > > -#define SUN8I_MIXER_BLEND_CK_MAX(x) (0x10c0 + 0x04 * (x)) > > > -#define SUN8I_MIXER_BLEND_CK_MIN(x) (0x10e0 + 0x04 * (x)) > > > -#define SUN8I_MIXER_BLEND_OUTCTL 0x10fc > > > +#define DE2_BLD_BASE 0x1000 > > > +#define DE2_CH_BASE 0x2000 > > > +#define DE2_CH_SIZE 0x1000 > > > + > > > +#define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > > > +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * > > > (x)) > > > +#define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * > > > (x)) > > > +#define SUN8I_MIXER_BLEND_ATTR_COORD(base, x) ((base) + 0xC + 0x10 * > > > (x)) > > > > Nit: Use lowercase for '0xC' to be consistent. > > > > > +#define SUN8I_MIXER_BLEND_ROUTE(base) ((base) + 0x80) > > > +#define SUN8I_MIXER_BLEND_PREMULTIPLY(base) ((base) + 0x84) > > > +#define SUN8I_MIXER_BLEND_BKCOLOR(base) ((base) + 0x88) > > > +#define SUN8I_MIXER_BLEND_OUTSIZE(base) ((base) + 0x8c) > > > +#define SUN8I_MIXER_BLEND_MODE(base, x) ((base) + 0x90 + > > > 0x04 * (x)) +#define SUN8I_MIXER_BLEND_CK_CTL(base) ((base) + > > > 0xb0) > > > +#define SUN8I_MIXER_BLEND_CK_CFG(base) ((base) + 0xb4) > > > +#define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * > > > (x)) +#define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + > > > 0x04 * (x)) +#define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + > > > 0xfc) > > > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > > > > [...] > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > > > b/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c index > > > 6bb2aa164c8e..c68eab8a748f 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_scaler.c > > > @@ -10,6 +10,7 @@ > > > > > > */ > > > > > > #include "sun8i_ui_scaler.h" > > > > > > +#include "sun8i_vi_scaler.h" > > > > > > static const u32 lan2coefftab16[240] = { > > > > > > 0x00004000, 0x00033ffe, 0x00063efc, 0x000a3bfb, > > > > > > @@ -88,6 +89,14 @@ static const u32 lan2coefftab16[240] = { > > > > > > 0x0b1c1603, 0x0d1c1502, 0x0e1d1401, 0x0f1d1301, > > > > > > }; > > > > > > +static inline u32 sun8i_ui_scaler_base(struct sun8i_mixer *mixer, int > > > channel) > > I recently saw a review comment stating one should not inline functions > > unless they are defined in header files. Otherwise the decision should > > be left up to the compiler. > > > > > +{ > > > + int vi_num = mixer->cfg->vi_num; > > > + > > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * vi_num + > > > + DE2_UI_SCALER_SIZE * (channel - vi_num); > > > +} > > > + > > > > > > static int sun8i_ui_scaler_coef_index(unsigned int step) > > > { > > > > > > unsigned int scale, int_part, float_part; > > > > [...] > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index > > > d3f1acb234b7..8697afc36023 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > @@ -833,6 +833,11 @@ static const u32 bicubic4coefftab32[480] = { > > > > > > 0x1012110d, 0x1012110d, 0x1013110c, 0x1013110c, > > > > > > }; > > > > > > +static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int > > > channel) +{ > > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > +} > > > + > > > > This one as well. > > > > > static int sun8i_vi_scaler_coef_index(unsigned int step) > > > { > > > > > > unsigned int scale, int_part, float_part; > > > > > > @@ -857,7 +862,7 @@ static int sun8i_vi_scaler_coef_index(unsigned int > > > step)> > > > } > > > > > > } > > > > > > -static void sun8i_vi_scaler_set_coeff(struct regmap *map, int layer, > > > +static void sun8i_vi_scaler_set_coeff(struct regmap *map, u32 base, > > > > > > u32 hstep, u32 vstep, > > > const struct drm_format_info > > > *format) > > > > This is the only instance where a function's "layer" parameter was changed > > to "base". It would be nice if it were consistent. > > Why not? Caller already have base address calculated, so it poses no overhead. > Additionally, sun8i_vi_scaler_set_coeff() doesn't have struct sun8i_mixer > *mixer parameter, which would allow calculating base address based on layer > id. So, you can chose between existing variant or adding additional parameter > for no real reason. Thanks for the explanation. Yeah, then the current patch makes sense. ChenYu
On Mon, Sep 24, 2018 at 3:51 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a): > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > wrote: > > > Display Engine 3 is an upgrade of DE2 with new features like support for > > > 10 bit color formats and support for AFBC. > > > > > > Most of DE2 code works with DE3, except some small details. > > > > > > Add support for it. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_csc.c | 96 +++++++++++++++++++++++-- > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 17 ++++- > > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 21 +++++- > > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 8 ++- > > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.h | 1 + > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 8 +++ > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 2 + > > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++- > > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++ > > > 9 files changed, 197 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c > > > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c > > > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = { > > > > > > 0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A, > > > > > > }; > > > > > > +/* > > > + * DE3 has a bit different CSC units. Factors are in two's complement > > > format. + * First three have 17 bits for fractinal part and last two 2 > > > bits. First + * three values in each line are multiplication factor, 4th > > > is difference, + * which is subtracted from the input value before the > > > multiplication and + * last value is constant, which is added at the end. > > > + * > > > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0 > > > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1 > > > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2 > > > + * > > > + * Please note that above formula is true only for Blender CSC. Other DE3 > > > CSC + * units takes only positive value for difference. From what can be > > > deducted + * from BSP driver code, those units probably automatically > > > assume that + * difference has to be subtracted. > > > + */ > > > +static const u32 yuv2rgb_de3[] = { > > > + 0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000, > > > + 0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000, > > > + 0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000, > > > +}; > > > + > > > +static const u32 yvu2rgb_de3[] = { > > > + 0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000, > > > + 0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000, > > > + 0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000, > > > +}; > > > + > > > > > > static void sun8i_csc_set_coefficients(struct regmap *map, u32 base, > > > > > > enum sun8i_csc_mode mode) > > > > > > { > > > > > > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap > > > *map, u32 base,> > > > } > > > > > > } > > > > > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int > > > layer, > > > + enum sun8i_csc_mode mode) > > > +{ > > > + const u32 *table; > > > + int i, j; > > > + > > > + switch (mode) { > > > + case SUN8I_CSC_MODE_YUV2RGB: > > > + table = yuv2rgb_de3; > > > + break; > > > + case SUN8I_CSC_MODE_YVU2RGB: > > > + table = yvu2rgb_de3; > > > + break; > > > + default: > > > + DRM_WARN("Wrong CSC mode specified.\n"); > > > + return; > > > + } > > > + > > > + for (i = 0; i < 3; i++) { > > > + for (j = 0; j < 3; j++) > > > + regmap_write(map, > > > + > > > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, + > > > layer, i, j), + > > > table[i * 5 + j]); > > > > Given that the first three values occupy contiguous addresses, > > you can use regmap_bulk_write() here. > > > > > + regmap_write(map, > > > + SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE, > > > + layer, i), > > > + SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 + > > > 3], + table[i > > > * 5 + 4])); > > Nit: Using a two-dimension array might make it easier to read. > > > > > + } > > > +} > > > + > > > > > > static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable) > > > { > > > > > > u32 val; > > > > > > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32 > > > base, bool enable)> > > > regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN, > > > val); > > > > > > } > > > > > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool > > > enable) +{ > > > + u32 val, mask; > > > + > > > + mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer); > > > + > > > + if (enable) > > > + val = mask; > > > + else > > > + val = 0; > > > + > > > + regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE), > > > + mask, val); > > > +} > > > + > > > > > > void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int > > > layer, > > > > > > enum sun8i_csc_mode mode) > > > > > > { > > > > > > - u32 base; > > > + if (!mixer->cfg->is_de3) { > > > + u32 base; > > > > You could rewrite this as > > > > if (mixer->cfg->is_de3) { > > sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > > layer, mode); > > return; > > } > > > > That way you don't need to change the indentation on the existing lines. > > I suppose this is more of a personal preference. The downside is the control > > flow is slightly more complicated. > > > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > > > - sun8i_csc_set_coefficients(mixer->engine.regs, base, mode); > > > + sun8i_csc_set_coefficients(mixer->engine.regs, base, > > > mode); > > > + } else { > > > + sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > > > + layer, mode); > > > + } > > > > > > } > > > > > > void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool > > > enable) { > > > > > > - u32 base; > > > + if (!mixer->cfg->is_de3) { > > > + u32 base; > > > > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > > > - sun8i_csc_enable(mixer->engine.regs, base, enable); > > > + sun8i_csc_enable(mixer->engine.regs, base, enable); > > > + } else { > > > + sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable); > > > + } > > > > > > } > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev, > > > struct device *master,> > > > base = sun8i_blender_base(mixer); > > > > > > /* Reset the registers */ > > > > > > - for (i = 0x0; i < 0x20000; i += 4) > > > - regmap_write(mixer->engine.regs, i, 0); > > > + if (mixer->cfg->is_de3) { > > > + for (i = 0x0; i < 0x3000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0x20000; i < 0x40000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0x70000; i < 0x88000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0xa0000; i < 0xb0000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0xd0000; i < 0xe0000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > > Could you use the macros *_BASE and *_SIZE here? That would make > > it more obviously what parts you're clearing. > > Ok, but where should I define them? In sun8i-mixer.h? If it's only used here you could put them just above this function. > > > > The last offset, 0xd0000, isn't even defined in the DE3 spec. > > It is, check chapter 2.3.2, table 2-3. I must have read something incorrectly. I see it now. Thanks. ChenYu > > > > > + } else { > > > + for (i = 0x0; i < 0x20000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + } > > > > > > /* Enable the mixer */ > > > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL, > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > @@ -33,6 +33,10 @@ > > > > > > #define DE2_CH_BASE 0x2000 > > > #define DE2_CH_SIZE 0x1000 > > > > > > +#define DE3_BLD_BASE 0x0800 > > > +#define DE3_CH_BASE 0x1000 > > > +#define DE3_CH_SIZE 0x0800 > > > + > > > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > > > #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * > > > (x)) > > > #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * > > > (x)) > > > > > > @@ -47,6 +51,11 @@ > > > > > > #define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * > > > (x)) > > > #define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + 0x04 * > > > (x)) > > > #define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + 0xfc) > > > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL(base) ((base) + 0x100) > > > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \ > > > + ((base) + 0x110 + (layer) * 0x30 + (x) * 0x10 + 4 * (y)) > > > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \ > > > + ((base) + 0x110 + (layer) * 0x30 + (i) * 0x10 + 0x0c) > > > > Should these have a DE3 prefix? > > Ok for all prefix comments. > > > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > > > > > > @@ -61,6 +70,9 @@ > > > > > > #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED BIT(1) > > > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch) BIT(ch) > > > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c) (((d) << 16) | ((c) & > > > 0xffff)) + > > > > Same here. > > > > > #define SUN8I_MIXER_FBFMT_ARGB8888 0 > > > #define SUN8I_MIXER_FBFMT_ABGR8888 1 > > > #define SUN8I_MIXER_FBFMT_RGBA8888 2 > > > > [...] > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > > @@ -30,6 +30,8 @@ > > > > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE BIT(15) > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET 8 > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK GENMASK(12, 8) > > > > > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK GENMASK(31, 24) > > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x) ((x) << > > > 24) > > > > DE3 prefix? > > > > > struct sun8i_mixer; > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index > > > 8697afc36023..22e1ed5cd7a4 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = { > > > > > > static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int > > > channel) { > > > > > > - return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > + if (mixer->cfg->is_de3) > > > + return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel; > > > + else > > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > > > > } > > > > > > static int sun8i_vi_scaler_coef_index(unsigned int step) > > > > > > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, > > > int layer,> > > > cvphase = vphase; > > > > > > } > > > > > > + if (mixer->cfg->is_de3) { > > > + u32 val; > > > + > > > + if (format->hsub == 1 && format->vsub == 1) > > > + val = SUN8I_SCALER_VSU_SCALE_MODE_UI; > > > + else > > > + val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL; > > > + > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_SCALE_MODE(base), val); > > > > The remaining settings seem to be related to the edge detection scaling > > method. Since you aren't supporting it, are they necessary? > > Didn't really tried without them IIRC. I will for next revision. > > Best regards, > Jernej > > > > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_DIR_THR(base), > > > + SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) | > > > + SUN8I_SCALER_VSU_ZERO_DIR_THR(0) | > > > + SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) | > > > + SUN8I_SCALER_VSU_VERT_DIR_THR(1)); > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_EDGE_THR(base), > > > + SUN8I_SCALER_VSU_EDGE_SHIFT(8) | > > > + SUN8I_SCALER_VSU_EDGE_OFFSET(0)); > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0); > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_ANGLE_THR(base), > > > + SUN8I_SCALER_VSU_ANGLE_SHIFT(2) | > > > + SUN8I_SCALER_VSU_ANGLE_OFFSET(0)); > > > + } > > > + > > > > > > regmap_write(mixer->engine.regs, > > > > > > SUN8I_SCALER_VSU_OUTSIZE(base), outsize); > > > > > > regmap_write(mixer->engine.regs, > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index > > > cd015405f66d..c322f5652481 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > > @@ -15,6 +15,9 @@ > > > > > > #define DE2_VI_SCALER_BASE 0x20000 > > > #define DE2_VI_SCALER_SIZE 0x20000 > > > > > > +#define DE3_VI_SCALER_BASE 0x20000 > > > +#define DE3_VI_SCALER_SIZE 0x08000 > > > + > > > > > > /* this two macros assumes 16 fractional bits which is standard in DRM */ > > > #define SUN8I_VI_SCALER_SCALE_MIN 1 > > > #define SUN8I_VI_SCALER_SCALE_MAX ((1UL << 20) - 1) > > > > > > @@ -25,6 +28,11 @@ > > > > > > #define SUN8I_VI_SCALER_SIZE(w, h) (((h) - 1) << 16 | ((w) - > > > 1)) > > > > > > #define SUN8I_SCALER_VSU_CTRL(base) ((base) + 0x0) > > > > > > +#define SUN8I_SCALER_VSU_SCALE_MODE(base) ((base) + 0x10) > > > +#define SUN8I_SCALER_VSU_DIR_THR(base) ((base) + 0x20) > > > +#define SUN8I_SCALER_VSU_EDGE_THR(base) ((base) + 0x24) > > > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base) ((base) + 0x28) > > > +#define SUN8I_SCALER_VSU_ANGLE_THR(base) ((base) + 0x2c) > > > > DE3 prefix for the new ones please. > > > > > #define SUN8I_SCALER_VSU_OUTSIZE(base) ((base) + 0x40) > > > #define SUN8I_SCALER_VSU_YINSIZE(base) ((base) + 0x80) > > > #define SUN8I_SCALER_VSU_YHSTEP(base) ((base) + 0x88) > > > > > > @@ -46,6 +54,21 @@ > > > > > > #define SUN8I_SCALER_VSU_CTRL_EN BIT(0) > > > #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY BIT(4) > > > > > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x) (((x) << 24) & 0xFF) > > > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x) (((x) << 16) & 0xFF) > > > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x) (((x) << 8) & 0xFF) > > > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x) ((x) & 0xFF) > > > + > > > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI 0 > > > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL 1 > > > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE 2 > > > + > > > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x) (((x) << 16) & 0xF) > > > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x) ((x) & 0xFF) > > > + > > > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x) (((x) << 16) & > > > 0xF) > > > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x) ((x) & 0xFF) > > > + > > > > Same here. > > > > Regards > > ChenYu > > > > > void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool > > > enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int > > > layer, > > > > > > u32 src_w, u32 src_h, u32 dst_w, u32 dst_h, > > > > > > -- > > > 2.18.0 > > > >
Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a): > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > Display Engine 3 is an upgrade of DE2 with new features like support for > > 10 bit color formats and support for AFBC. > > > > Most of DE2 code works with DE3, except some small details. > > > > Add support for it. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_csc.c | 96 +++++++++++++++++++++++-- > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 17 ++++- > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 21 +++++- > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 8 ++- > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.h | 1 + > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 8 +++ > > drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 2 + > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++- > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++ > > 9 files changed, 197 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c > > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c > > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = { > > > > 0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A, > > > > }; > > > > +/* > > + * DE3 has a bit different CSC units. Factors are in two's complement > > format. + * First three have 17 bits for fractinal part and last two 2 > > bits. First + * three values in each line are multiplication factor, 4th > > is difference, + * which is subtracted from the input value before the > > multiplication and + * last value is constant, which is added at the end. > > + * > > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0 > > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1 > > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2 > > + * > > + * Please note that above formula is true only for Blender CSC. Other DE3 > > CSC + * units takes only positive value for difference. From what can be > > deducted + * from BSP driver code, those units probably automatically > > assume that + * difference has to be subtracted. > > + */ > > +static const u32 yuv2rgb_de3[] = { > > + 0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000, > > + 0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000, > > + 0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000, > > +}; > > + > > +static const u32 yvu2rgb_de3[] = { > > + 0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000, > > + 0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000, > > + 0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000, > > +}; > > + > > > > static void sun8i_csc_set_coefficients(struct regmap *map, u32 base, > > > > enum sun8i_csc_mode mode) > > > > { > > > > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap > > *map, u32 base,> > > } > > > > } > > > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int > > layer, > > + enum sun8i_csc_mode mode) > > +{ > > + const u32 *table; > > + int i, j; > > + > > + switch (mode) { > > + case SUN8I_CSC_MODE_YUV2RGB: > > + table = yuv2rgb_de3; > > + break; > > + case SUN8I_CSC_MODE_YVU2RGB: > > + table = yvu2rgb_de3; > > + break; > > + default: > > + DRM_WARN("Wrong CSC mode specified.\n"); > > + return; > > + } > > + > > + for (i = 0; i < 3; i++) { > > + for (j = 0; j < 3; j++) > > + regmap_write(map, > > + > > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, + > > layer, i, j), + > > table[i * 5 + j]); > > Given that the first three values occupy contiguous addresses, > you can use regmap_bulk_write() here. Do you mind if I rework table in a way that directly fits in registers and use regmap_bulk_write() on whole table? This means I must combine two values into one for "const" register, but I would explain it better in a comment. It would be most elegant solution, one call to regmap_bulk_write() instead of two for loops and some arithmetics. I guess macro for "const" part can stay as documentation. > > > + regmap_write(map, > > + SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE, > > + layer, i), > > + SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 + > > 3], + table[i > > * 5 + 4])); > Nit: Using a two-dimension array might make it easier to read. > > > + } > > +} > > + > > > > static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable) > > { > > > > u32 val; > > > > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32 > > base, bool enable)> > > regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN, > > val); > > > > } > > > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool > > enable) +{ > > + u32 val, mask; > > + > > + mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer); > > + > > + if (enable) > > + val = mask; > > + else > > + val = 0; > > + > > + regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE), > > + mask, val); > > +} > > + > > > > void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int > > layer, > > > > enum sun8i_csc_mode mode) > > > > { > > > > - u32 base; > > + if (!mixer->cfg->is_de3) { > > + u32 base; > > You could rewrite this as > > if (mixer->cfg->is_de3) { > sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > layer, mode); > return; > } > > That way you don't need to change the indentation on the existing lines. > I suppose this is more of a personal preference. The downside is the control > flow is slightly more complicated. > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > - sun8i_csc_set_coefficients(mixer->engine.regs, base, mode); > > + sun8i_csc_set_coefficients(mixer->engine.regs, base, > > mode); > > + } else { > > + sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > > + layer, mode); > > + } > > > > } > > > > void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool > > enable) { > > > > - u32 base; > > + if (!mixer->cfg->is_de3) { > > + u32 base; > > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > - sun8i_csc_enable(mixer->engine.regs, base, enable); > > + sun8i_csc_enable(mixer->engine.regs, base, enable); > > + } else { > > + sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable); > > + } > > > > } > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev, > > struct device *master,> > > base = sun8i_blender_base(mixer); > > > > /* Reset the registers */ > > > > - for (i = 0x0; i < 0x20000; i += 4) > > - regmap_write(mixer->engine.regs, i, 0); > > + if (mixer->cfg->is_de3) { > > + for (i = 0x0; i < 0x3000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0x20000; i < 0x40000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0x70000; i < 0x88000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0xa0000; i < 0xb0000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + for (i = 0xd0000; i < 0xe0000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > Could you use the macros *_BASE and *_SIZE here? That would make > it more obviously what parts you're clearing. > > The last offset, 0xd0000, isn't even defined in the DE3 spec. > > > + } else { > > + for (i = 0x0; i < 0x20000; i += 4) > > + regmap_write(mixer->engine.regs, i, 0); > > + } > > > > /* Enable the mixer */ > > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL, > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44 > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > > @@ -33,6 +33,10 @@ > > > > #define DE2_CH_BASE 0x2000 > > #define DE2_CH_SIZE 0x1000 > > > > +#define DE3_BLD_BASE 0x0800 > > +#define DE3_CH_BASE 0x1000 > > +#define DE3_CH_SIZE 0x0800 > > + > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > > #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * > > (x)) > > #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * > > (x)) > > > > @@ -47,6 +51,11 @@ > > > > #define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * > > (x)) > > #define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + 0x04 * > > (x)) > > #define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + 0xfc) > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL(base) ((base) + 0x100) > > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \ > > + ((base) + 0x110 + (layer) * 0x30 + (x) * 0x10 + 4 * (y)) > > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \ > > + ((base) + 0x110 + (layer) * 0x30 + (i) * 0x10 + 0x0c) > > Should these have a DE3 prefix? How do you want this to look like? SUN50I_DE3_MIXER_BLEND_CSC_CONST? SUN8I prefix doesn't fit for DE3, but as usual, there is a chance that AW releases 32 bit SoC with DE3. At the end, it would be better to rename everything to have just DE2 or DE3 prefix without mentioning any SoC family. After all, DE2 is also supported on sun50i family, which makes any family name in DE2/3 macros confusing... Do you think I can squeeze dropping SUN8I prefix to a previous patch, where macros are reworked anyway? Best regards, Jernej > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > > > > @@ -61,6 +70,9 @@ > > > > #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED BIT(1) > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch) BIT(ch) > > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c) (((d) << 16) | ((c) & > > 0xffff)) + > > Same here. > > > #define SUN8I_MIXER_FBFMT_ARGB8888 0 > > #define SUN8I_MIXER_FBFMT_ABGR8888 1 > > #define SUN8I_MIXER_FBFMT_RGBA8888 2 > > [...] > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea > > 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > @@ -30,6 +30,8 @@ > > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE BIT(15) > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET 8 > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK GENMASK(12, 8) > > > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK GENMASK(31, 24) > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x) ((x) << > > 24) > > DE3 prefix? > > > struct sun8i_mixer; > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index > > 8697afc36023..22e1ed5cd7a4 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = { > > > > static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int > > channel) { > > > > - return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > + if (mixer->cfg->is_de3) > > + return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel; > > + else > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > > } > > > > static int sun8i_vi_scaler_coef_index(unsigned int step) > > > > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, > > int layer,> > > cvphase = vphase; > > > > } > > > > + if (mixer->cfg->is_de3) { > > + u32 val; > > + > > + if (format->hsub == 1 && format->vsub == 1) > > + val = SUN8I_SCALER_VSU_SCALE_MODE_UI; > > + else > > + val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL; > > + > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_SCALE_MODE(base), val); > > The remaining settings seem to be related to the edge detection scaling > method. Since you aren't supporting it, are they necessary? > > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_DIR_THR(base), > > + SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) | > > + SUN8I_SCALER_VSU_ZERO_DIR_THR(0) | > > + SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) | > > + SUN8I_SCALER_VSU_VERT_DIR_THR(1)); > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_EDGE_THR(base), > > + SUN8I_SCALER_VSU_EDGE_SHIFT(8) | > > + SUN8I_SCALER_VSU_EDGE_OFFSET(0)); > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0); > > + regmap_write(mixer->engine.regs, > > + SUN8I_SCALER_VSU_ANGLE_THR(base), > > + SUN8I_SCALER_VSU_ANGLE_SHIFT(2) | > > + SUN8I_SCALER_VSU_ANGLE_OFFSET(0)); > > + } > > + > > > > regmap_write(mixer->engine.regs, > > > > SUN8I_SCALER_VSU_OUTSIZE(base), outsize); > > > > regmap_write(mixer->engine.regs, > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index > > cd015405f66d..c322f5652481 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > @@ -15,6 +15,9 @@ > > > > #define DE2_VI_SCALER_BASE 0x20000 > > #define DE2_VI_SCALER_SIZE 0x20000 > > > > +#define DE3_VI_SCALER_BASE 0x20000 > > +#define DE3_VI_SCALER_SIZE 0x08000 > > + > > > > /* this two macros assumes 16 fractional bits which is standard in DRM */ > > #define SUN8I_VI_SCALER_SCALE_MIN 1 > > #define SUN8I_VI_SCALER_SCALE_MAX ((1UL << 20) - 1) > > > > @@ -25,6 +28,11 @@ > > > > #define SUN8I_VI_SCALER_SIZE(w, h) (((h) - 1) << 16 | ((w) - > > 1)) > > > > #define SUN8I_SCALER_VSU_CTRL(base) ((base) + 0x0) > > > > +#define SUN8I_SCALER_VSU_SCALE_MODE(base) ((base) + 0x10) > > +#define SUN8I_SCALER_VSU_DIR_THR(base) ((base) + 0x20) > > +#define SUN8I_SCALER_VSU_EDGE_THR(base) ((base) + 0x24) > > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base) ((base) + 0x28) > > +#define SUN8I_SCALER_VSU_ANGLE_THR(base) ((base) + 0x2c) > > DE3 prefix for the new ones please. > > > #define SUN8I_SCALER_VSU_OUTSIZE(base) ((base) + 0x40) > > #define SUN8I_SCALER_VSU_YINSIZE(base) ((base) + 0x80) > > #define SUN8I_SCALER_VSU_YHSTEP(base) ((base) + 0x88) > > > > @@ -46,6 +54,21 @@ > > > > #define SUN8I_SCALER_VSU_CTRL_EN BIT(0) > > #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY BIT(4) > > > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x) (((x) << 24) & 0xFF) > > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x) (((x) << 16) & 0xFF) > > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x) (((x) << 8) & 0xFF) > > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x) ((x) & 0xFF) > > + > > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI 0 > > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL 1 > > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE 2 > > + > > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x) (((x) << 16) & 0xF) > > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x) ((x) & 0xFF) > > + > > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x) (((x) << 16) & > > 0xF) > > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x) ((x) & 0xFF) > > + > > Same here. > > Regards > ChenYu > > > void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool > > enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int > > layer, > > > > u32 src_w, u32 src_h, u32 dst_w, u32 dst_h, > > > > -- > > 2.18.0
On Sat, Oct 6, 2018 at 1:51 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a): > > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> > wrote: > > > Display Engine 3 is an upgrade of DE2 with new features like support for > > > 10 bit color formats and support for AFBC. > > > > > > Most of DE2 code works with DE3, except some small details. > > > > > > Add support for it. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_csc.c | 96 +++++++++++++++++++++++-- > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 17 ++++- > > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 21 +++++- > > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 8 ++- > > > drivers/gpu/drm/sun4i/sun8i_ui_scaler.h | 1 + > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 8 +++ > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.h | 2 + > > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++- > > > drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++ > > > 9 files changed, 197 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c > > > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c > > > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = { > > > > > > 0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A, > > > > > > }; > > > > > > +/* > > > + * DE3 has a bit different CSC units. Factors are in two's complement > > > format. + * First three have 17 bits for fractinal part and last two 2 > > > bits. First + * three values in each line are multiplication factor, 4th > > > is difference, + * which is subtracted from the input value before the > > > multiplication and + * last value is constant, which is added at the end. > > > + * > > > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0 > > > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1 > > > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2 > > > + * > > > + * Please note that above formula is true only for Blender CSC. Other DE3 > > > CSC + * units takes only positive value for difference. From what can be > > > deducted + * from BSP driver code, those units probably automatically > > > assume that + * difference has to be subtracted. > > > + */ > > > +static const u32 yuv2rgb_de3[] = { > > > + 0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000, > > > + 0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000, > > > + 0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000, > > > +}; > > > + > > > +static const u32 yvu2rgb_de3[] = { > > > + 0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000, > > > + 0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000, > > > + 0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000, > > > +}; > > > + > > > > > > static void sun8i_csc_set_coefficients(struct regmap *map, u32 base, > > > > > > enum sun8i_csc_mode mode) > > > > > > { > > > > > > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap > > > *map, u32 base,> > > > } > > > > > > } > > > > > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int > > > layer, > > > + enum sun8i_csc_mode mode) > > > +{ > > > + const u32 *table; > > > + int i, j; > > > + > > > + switch (mode) { > > > + case SUN8I_CSC_MODE_YUV2RGB: > > > + table = yuv2rgb_de3; > > > + break; > > > + case SUN8I_CSC_MODE_YVU2RGB: > > > + table = yvu2rgb_de3; > > > + break; > > > + default: > > > + DRM_WARN("Wrong CSC mode specified.\n"); > > > + return; > > > + } > > > + > > > + for (i = 0; i < 3; i++) { > > > + for (j = 0; j < 3; j++) > > > + regmap_write(map, > > > + > > > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, + > > > layer, i, j), + > > > table[i * 5 + j]); > > > > Given that the first three values occupy contiguous addresses, > > you can use regmap_bulk_write() here. > > Do you mind if I rework table in a way that directly fits in registers and use > regmap_bulk_write() on whole table? This means I must combine two values into > one for "const" register, but I would explain it better in a comment. It would > be most elegant solution, one call to regmap_bulk_write() instead of two for > loops and some arithmetics. With proper comments I see no problem with it. The constants are hexidecimal and only loaded once. It's unlikely others would use it any other way. > I guess macro for "const" part can stay as documentation. I agree. > > > > > > + regmap_write(map, > > > + SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE, > > > + layer, i), > > > + SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 + > > > 3], + table[i > > > * 5 + 4])); > > Nit: Using a two-dimension array might make it easier to read. > > > > > + } > > > +} > > > + > > > > > > static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable) > > > { > > > > > > u32 val; > > > > > > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32 > > > base, bool enable)> > > > regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN, > > > val); > > > > > > } > > > > > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool > > > enable) +{ > > > + u32 val, mask; > > > + > > > + mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer); > > > + > > > + if (enable) > > > + val = mask; > > > + else > > > + val = 0; > > > + > > > + regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE), > > > + mask, val); > > > +} > > > + > > > > > > void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int > > > layer, > > > > > > enum sun8i_csc_mode mode) > > > > > > { > > > > > > - u32 base; > > > + if (!mixer->cfg->is_de3) { > > > + u32 base; > > > > You could rewrite this as > > > > if (mixer->cfg->is_de3) { > > sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > > layer, mode); > > return; > > } > > > > That way you don't need to change the indentation on the existing lines. > > I suppose this is more of a personal preference. The downside is the control > > flow is slightly more complicated. > > > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > > > - sun8i_csc_set_coefficients(mixer->engine.regs, base, mode); > > > + sun8i_csc_set_coefficients(mixer->engine.regs, base, > > > mode); > > > + } else { > > > + sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, > > > + layer, mode); > > > + } > > > > > > } > > > > > > void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool > > > enable) { > > > > > > - u32 base; > > > + if (!mixer->cfg->is_de3) { > > > + u32 base; > > > > > > - base = ccsc_base[mixer->cfg->ccsc][layer]; > > > + base = ccsc_base[mixer->cfg->ccsc][layer]; > > > > > > - sun8i_csc_enable(mixer->engine.regs, base, enable); > > > + sun8i_csc_enable(mixer->engine.regs, base, enable); > > > + } else { > > > + sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable); > > > + } > > > > > > } > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev, > > > struct device *master,> > > > base = sun8i_blender_base(mixer); > > > > > > /* Reset the registers */ > > > > > > - for (i = 0x0; i < 0x20000; i += 4) > > > - regmap_write(mixer->engine.regs, i, 0); > > > + if (mixer->cfg->is_de3) { > > > + for (i = 0x0; i < 0x3000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0x20000; i < 0x40000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0x70000; i < 0x88000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0xa0000; i < 0xb0000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + for (i = 0xd0000; i < 0xe0000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > > Could you use the macros *_BASE and *_SIZE here? That would make > > it more obviously what parts you're clearing. > > > > The last offset, 0xd0000, isn't even defined in the DE3 spec. > > > > > + } else { > > > + for (i = 0x0; i < 0x20000; i += 4) > > > + regmap_write(mixer->engine.regs, i, 0); > > > + } > > > > > > /* Enable the mixer */ > > > regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL, > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h > > > @@ -33,6 +33,10 @@ > > > > > > #define DE2_CH_BASE 0x2000 > > > #define DE2_CH_SIZE 0x1000 > > > > > > +#define DE3_BLD_BASE 0x0800 > > > +#define DE3_CH_BASE 0x1000 > > > +#define DE3_CH_SIZE 0x0800 > > > + > > > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL(base) ((base) + 0) > > > #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * > > > (x)) > > > #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * > > > (x)) > > > > > > @@ -47,6 +51,11 @@ > > > > > > #define SUN8I_MIXER_BLEND_CK_MAX(base, x) ((base) + 0xc0 + 0x04 * > > > (x)) > > > #define SUN8I_MIXER_BLEND_CK_MIN(base, x) ((base) + 0xe0 + 0x04 * > > > (x)) > > > #define SUN8I_MIXER_BLEND_OUTCTL(base) ((base) + 0xfc) > > > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL(base) ((base) + 0x100) > > > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \ > > > + ((base) + 0x110 + (layer) * 0x30 + (x) * 0x10 + 4 * (y)) > > > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \ > > > + ((base) + 0x110 + (layer) * 0x30 + (i) * 0x10 + 0x0c) > > > > Should these have a DE3 prefix? > > How do you want this to look like? > > SUN50I_DE3_MIXER_BLEND_CSC_CONST? > > SUN8I prefix doesn't fit for DE3, but as usual, there is a chance that AW > releases 32 bit SoC with DE3. > > At the end, it would be better to rename everything to have just DE2 or DE3 > prefix without mentioning any SoC family. After all, DE2 is also supported on > sun50i family, which makes any family name in DE2/3 macros confusing... Probably SUNXI_DEx_MIXER_foo or just DEx_MIXER_foo ... Since it's contained in the driver I think the latter would suffice. > Do you think I can squeeze dropping SUN8I prefix to a previous patch, where > macros are reworked anyway? Given there's a lot of them, and you're only changing about half of them in that patch, I suggest you have a separate patch to do all the renaming. ChenYu > Best regards, > Jernej > > > > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK GENMASK(12, 8) > > > #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe) BIT(8 + pipe) > > > > > > @@ -61,6 +70,9 @@ > > > > > > #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED BIT(1) > > > > > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch) BIT(ch) > > > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c) (((d) << 16) | ((c) & > > > 0xffff)) + > > > > Same here. > > > > > #define SUN8I_MIXER_FBFMT_ARGB8888 0 > > > #define SUN8I_MIXER_FBFMT_ABGR8888 1 > > > #define SUN8I_MIXER_FBFMT_RGBA8888 2 > > > > [...] > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h > > > @@ -30,6 +30,8 @@ > > > > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE BIT(15) > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET 8 > > > #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK GENMASK(12, 8) > > > > > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK GENMASK(31, 24) > > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x) ((x) << > > > 24) > > > > DE3 prefix? > > > > > struct sun8i_mixer; > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index > > > 8697afc36023..22e1ed5cd7a4 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c > > > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = { > > > > > > static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int > > > channel) { > > > > > > - return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > + if (mixer->cfg->is_de3) > > > + return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel; > > > + else > > > + return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel; > > > > > > } > > > > > > static int sun8i_vi_scaler_coef_index(unsigned int step) > > > > > > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, > > > int layer,> > > > cvphase = vphase; > > > > > > } > > > > > > + if (mixer->cfg->is_de3) { > > > + u32 val; > > > + > > > + if (format->hsub == 1 && format->vsub == 1) > > > + val = SUN8I_SCALER_VSU_SCALE_MODE_UI; > > > + else > > > + val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL; > > > + > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_SCALE_MODE(base), val); > > > > The remaining settings seem to be related to the edge detection scaling > > method. Since you aren't supporting it, are they necessary? > > > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_DIR_THR(base), > > > + SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) | > > > + SUN8I_SCALER_VSU_ZERO_DIR_THR(0) | > > > + SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) | > > > + SUN8I_SCALER_VSU_VERT_DIR_THR(1)); > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_EDGE_THR(base), > > > + SUN8I_SCALER_VSU_EDGE_SHIFT(8) | > > > + SUN8I_SCALER_VSU_EDGE_OFFSET(0)); > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0); > > > + regmap_write(mixer->engine.regs, > > > + SUN8I_SCALER_VSU_ANGLE_THR(base), > > > + SUN8I_SCALER_VSU_ANGLE_SHIFT(2) | > > > + SUN8I_SCALER_VSU_ANGLE_OFFSET(0)); > > > + } > > > + > > > > > > regmap_write(mixer->engine.regs, > > > > > > SUN8I_SCALER_VSU_OUTSIZE(base), outsize); > > > > > > regmap_write(mixer->engine.regs, > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index > > > cd015405f66d..c322f5652481 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h > > > @@ -15,6 +15,9 @@ > > > > > > #define DE2_VI_SCALER_BASE 0x20000 > > > #define DE2_VI_SCALER_SIZE 0x20000 > > > > > > +#define DE3_VI_SCALER_BASE 0x20000 > > > +#define DE3_VI_SCALER_SIZE 0x08000 > > > + > > > > > > /* this two macros assumes 16 fractional bits which is standard in DRM */ > > > #define SUN8I_VI_SCALER_SCALE_MIN 1 > > > #define SUN8I_VI_SCALER_SCALE_MAX ((1UL << 20) - 1) > > > > > > @@ -25,6 +28,11 @@ > > > > > > #define SUN8I_VI_SCALER_SIZE(w, h) (((h) - 1) << 16 | ((w) - > > > 1)) > > > > > > #define SUN8I_SCALER_VSU_CTRL(base) ((base) + 0x0) > > > > > > +#define SUN8I_SCALER_VSU_SCALE_MODE(base) ((base) + 0x10) > > > +#define SUN8I_SCALER_VSU_DIR_THR(base) ((base) + 0x20) > > > +#define SUN8I_SCALER_VSU_EDGE_THR(base) ((base) + 0x24) > > > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base) ((base) + 0x28) > > > +#define SUN8I_SCALER_VSU_ANGLE_THR(base) ((base) + 0x2c) > > > > DE3 prefix for the new ones please. > > > > > #define SUN8I_SCALER_VSU_OUTSIZE(base) ((base) + 0x40) > > > #define SUN8I_SCALER_VSU_YINSIZE(base) ((base) + 0x80) > > > #define SUN8I_SCALER_VSU_YHSTEP(base) ((base) + 0x88) > > > > > > @@ -46,6 +54,21 @@ > > > > > > #define SUN8I_SCALER_VSU_CTRL_EN BIT(0) > > > #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY BIT(4) > > > > > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x) (((x) << 24) & 0xFF) > > > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x) (((x) << 16) & 0xFF) > > > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x) (((x) << 8) & 0xFF) > > > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x) ((x) & 0xFF) > > > + > > > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI 0 > > > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL 1 > > > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE 2 > > > + > > > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x) (((x) << 16) & 0xF) > > > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x) ((x) & 0xFF) > > > + > > > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x) (((x) << 16) & > > > 0xF) > > > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x) ((x) & 0xFF) > > > + > > > > Same here. > > > > Regards > > ChenYu > > > > > void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool > > > enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int > > > layer, > > > > > > u32 src_w, u32 src_h, u32 dst_w, u32 dst_h, > > > > > > -- > > > 2.18.0 > > > > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.