| Message ID | 20230423121232.1345909-6-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: bridge: samsung-dsim: Support variable clocking | expand |
On 23.04.2023 14:12, Adam Ford wrote: > The high-speed clock is hard-coded to the burst-clock > frequency specified in the device tree. However, when > using devices like certain bridge chips without burst mode > and varying resolutions and refresh rates, it may be > necessary to set the high-speed clock dynamically based > on the desired pixel clock for the connected device. > > This also removes the need to set a clock speed from > the device tree for non-burst mode operation, since the > pixel clock rate is the rate requested from the attached > device like an HDMI bridge chip. This should have no > impact for people using burst-mode and setting the burst > clock rate is still required for those users. > > Signed-off-by: Adam Ford <aford173@gmail.com> This one breaks Exynos-5433 based TM2e board with a DSI panel. > --- > drivers/gpu/drm/bridge/samsung-dsim.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > index f165483d5044..cea847b8e23c 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -657,11 +657,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, > > static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) > { > - unsigned long hs_clk, byte_clk, esc_clk; > + unsigned long hs_clk, byte_clk, esc_clk, pix_clk; > unsigned long esc_div; > u32 reg; > + struct drm_display_mode *m = &dsi->mode; > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > + > + /* m->clock is in KHz */ > + pix_clk = m->clock * 1000; > + > + /* Use burst_clk_rate for burst mode, otherwise use the pix_clk */ > + if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->burst_clk_rate) > + hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); > + else > + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); > > - hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); > if (!hs_clk) { > dev_err(dsi->dev, "failed to configure DSI PLL\n"); > return -EFAULT; > @@ -1800,10 +1810,11 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi) > return PTR_ERR(pll_clk); > } > > + /* If it doesn't exist, use pixel clock instead of failing */ > ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency", > &dsi->burst_clk_rate); > if (ret < 0) > - return ret; > + dsi->burst_clk_rate = 0; > > ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency", > &dsi->esc_clk_rate); Best regards
On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 23.04.2023 14:12, Adam Ford wrote: > > The high-speed clock is hard-coded to the burst-clock > > frequency specified in the device tree. However, when > > using devices like certain bridge chips without burst mode > > and varying resolutions and refresh rates, it may be > > necessary to set the high-speed clock dynamically based > > on the desired pixel clock for the connected device. > > > > This also removes the need to set a clock speed from > > the device tree for non-burst mode operation, since the > > pixel clock rate is the rate requested from the attached > > device like an HDMI bridge chip. This should have no > > impact for people using burst-mode and setting the burst > > clock rate is still required for those users. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > This one breaks Exynos-5433 based TM2e board with a DSI panel. Marek S, Thank you for testing! I knoiw there are several of us who appreciate your testing this since it's hard to know if something broke without hardware. Is there any way you can tell me if the flag is set to enable MIPI_DSI_MODE_VIDEO_BURST? I was trying to be diligent about not breaking your boards, but without your boards, it's difficult. The theory was that if MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the device tree, it would use the burst clock. As a fall-back I could just simply check for the presence of the burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and burst_clock_rate. > > > --- > > drivers/gpu/drm/bridge/samsung-dsim.c | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > > index f165483d5044..cea847b8e23c 100644 > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > > @@ -657,11 +657,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, > > > > static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) > > { > > - unsigned long hs_clk, byte_clk, esc_clk; > > + unsigned long hs_clk, byte_clk, esc_clk, pix_clk; > > unsigned long esc_div; > > u32 reg; > > + struct drm_display_mode *m = &dsi->mode; > > + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > + > > + /* m->clock is in KHz */ > > + pix_clk = m->clock * 1000; > > + > > + /* Use burst_clk_rate for burst mode, otherwise use the pix_clk */ > > + if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->burst_clk_rate) Would you be willing to test this if this line just read: if (dsi->burst_clk_rate) That would tell me if my fallback idea works. Thank you, adam > > + hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); > > + else > > + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); > > > > - hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); > > if (!hs_clk) { > > dev_err(dsi->dev, "failed to configure DSI PLL\n"); > > return -EFAULT; > > @@ -1800,10 +1810,11 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi) > > return PTR_ERR(pll_clk); > > } > > > > + /* If it doesn't exist, use pixel clock instead of failing */ > > ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency", > > &dsi->burst_clk_rate); > > if (ret < 0) > > - return ret; > > + dsi->burst_clk_rate = 0; > > > > ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency", > > &dsi->esc_clk_rate); > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
On 24.04.2023 12:00, Adam Ford wrote: > On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 23.04.2023 14:12, Adam Ford wrote: >>> The high-speed clock is hard-coded to the burst-clock >>> frequency specified in the device tree. However, when >>> using devices like certain bridge chips without burst mode >>> and varying resolutions and refresh rates, it may be >>> necessary to set the high-speed clock dynamically based >>> on the desired pixel clock for the connected device. >>> >>> This also removes the need to set a clock speed from >>> the device tree for non-burst mode operation, since the >>> pixel clock rate is the rate requested from the attached >>> device like an HDMI bridge chip. This should have no >>> impact for people using burst-mode and setting the burst >>> clock rate is still required for those users. >>> >>> Signed-off-by: Adam Ford <aford173@gmail.com> >> This one breaks Exynos-5433 based TM2e board with a DSI panel. > Marek S, > > Thank you for testing! I knoiw there are several of us who appreciate > your testing this since it's hard to know if something broke without > hardware. Is there any way you can tell me if the flag is set to > enable MIPI_DSI_MODE_VIDEO_BURST? TM2e board uses the DSI panel operated in command mode and handled by panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags is set there. I really have no idea if setting VIDEO_BURST would make sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks setting it? > I was trying to be diligent about not breaking your boards, but > without your boards, it's difficult. The theory was that if > MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the > device tree, it would use the burst clock. > > As a fall-back I could just simply check for the presence of the > burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and > burst_clock_rate. Maybe you should extend your check also for the MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense? > ... Best regards
On Fri, Apr 28, 2023 at 7:31 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 24.04.2023 12:00, Adam Ford wrote: > > On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 23.04.2023 14:12, Adam Ford wrote: > >>> The high-speed clock is hard-coded to the burst-clock > >>> frequency specified in the device tree. However, when > >>> using devices like certain bridge chips without burst mode > >>> and varying resolutions and refresh rates, it may be > >>> necessary to set the high-speed clock dynamically based > >>> on the desired pixel clock for the connected device. > >>> > >>> This also removes the need to set a clock speed from > >>> the device tree for non-burst mode operation, since the > >>> pixel clock rate is the rate requested from the attached > >>> device like an HDMI bridge chip. This should have no > >>> impact for people using burst-mode and setting the burst > >>> clock rate is still required for those users. > >>> > >>> Signed-off-by: Adam Ford <aford173@gmail.com> > >> This one breaks Exynos-5433 based TM2e board with a DSI panel. > > Marek S, > > > > Thank you for testing! I knoiw there are several of us who appreciate > > your testing this since it's hard to know if something broke without > > hardware. Is there any way you can tell me if the flag is set to > > enable MIPI_DSI_MODE_VIDEO_BURST? > > TM2e board uses the DSI panel operated in command mode and handled by > panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is > not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags > is set there. I really have no idea if setting VIDEO_BURST would make > sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks > setting it? > > > > I was trying to be diligent about not breaking your boards, but > > without your boards, it's difficult. The theory was that if > > MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the > > device tree, it would use the burst clock. > > > > As a fall-back I could just simply check for the presence of the > > burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and > > burst_clock_rate. > > Maybe you should extend your check also for the > MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense? Looking at some of the devices that might attach in the future, It appears that ti-sn65dsi86.c sets this flag. It's a display port bridge, so I would expect it to need a variable clock rate similar to how the HDMI bridge that I need works. I am concerned that I make the burst clock dependent on MIPI_DSI_CLOCK_NON_CONTINUOUS, it might break the Display Port bridge. I think it's better to just check if the samsung,burst-clock-frequency is present in the device tree and use it when present. If it's not present, then fall back to the pixel clock of the connected device. I looked at a bunch of Exynos parts, and it looks like they all use the samsung,burst-clock-frequency device tree setting. Is that true, or did I miss one? adam > > > ... > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
On 28.04.2023 15:35, Adam Ford wrote: > On Fri, Apr 28, 2023 at 7:31 AM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 24.04.2023 12:00, Adam Ford wrote: >>> On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski >>> <m.szyprowski@samsung.com> wrote: >>>> On 23.04.2023 14:12, Adam Ford wrote: >>>>> The high-speed clock is hard-coded to the burst-clock >>>>> frequency specified in the device tree. However, when >>>>> using devices like certain bridge chips without burst mode >>>>> and varying resolutions and refresh rates, it may be >>>>> necessary to set the high-speed clock dynamically based >>>>> on the desired pixel clock for the connected device. >>>>> >>>>> This also removes the need to set a clock speed from >>>>> the device tree for non-burst mode operation, since the >>>>> pixel clock rate is the rate requested from the attached >>>>> device like an HDMI bridge chip. This should have no >>>>> impact for people using burst-mode and setting the burst >>>>> clock rate is still required for those users. >>>>> >>>>> Signed-off-by: Adam Ford <aford173@gmail.com> >>>> This one breaks Exynos-5433 based TM2e board with a DSI panel. >>> Marek S, >>> >>> Thank you for testing! I knoiw there are several of us who appreciate >>> your testing this since it's hard to know if something broke without >>> hardware. Is there any way you can tell me if the flag is set to >>> enable MIPI_DSI_MODE_VIDEO_BURST? >> TM2e board uses the DSI panel operated in command mode and handled by >> panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is >> not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags >> is set there. I really have no idea if setting VIDEO_BURST would make >> sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks >> setting it? >> >> >>> I was trying to be diligent about not breaking your boards, but >>> without your boards, it's difficult. The theory was that if >>> MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the >>> device tree, it would use the burst clock. >>> >>> As a fall-back I could just simply check for the presence of the >>> burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and >>> burst_clock_rate. >> Maybe you should extend your check also for the >> MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense? > Looking at some of the devices that might attach in the future, It > appears that ti-sn65dsi86.c sets this flag. It's a display port > bridge, so I would expect it to need a variable clock rate similar to > how the HDMI bridge that I need works. I am concerned that I make the > burst clock dependent on MIPI_DSI_CLOCK_NON_CONTINUOUS, it might break > the Display Port bridge. > > I think it's better to just check if the samsung,burst-clock-frequency > is present in the device tree and use it when present. If it's not > present, then fall back to the pixel clock of the connected device. Right, this sounds rational. > I looked at a bunch of Exynos parts, and it looks like they all use > the samsung,burst-clock-frequency device tree setting. Is that true, > or did I miss one? That true. All Exynos based boards with DSI panels use constant DSI burst frequency defined in the device tree. Best regards
On Fri, Apr 28, 2023 at 9:04 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 28.04.2023 15:35, Adam Ford wrote: > > On Fri, Apr 28, 2023 at 7:31 AM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 24.04.2023 12:00, Adam Ford wrote: > >>> On Mon, Apr 24, 2023 at 3:25 AM Marek Szyprowski > >>> <m.szyprowski@samsung.com> wrote: > >>>> On 23.04.2023 14:12, Adam Ford wrote: > >>>>> The high-speed clock is hard-coded to the burst-clock > >>>>> frequency specified in the device tree. However, when > >>>>> using devices like certain bridge chips without burst mode > >>>>> and varying resolutions and refresh rates, it may be > >>>>> necessary to set the high-speed clock dynamically based > >>>>> on the desired pixel clock for the connected device. > >>>>> > >>>>> This also removes the need to set a clock speed from > >>>>> the device tree for non-burst mode operation, since the > >>>>> pixel clock rate is the rate requested from the attached > >>>>> device like an HDMI bridge chip. This should have no > >>>>> impact for people using burst-mode and setting the burst > >>>>> clock rate is still required for those users. > >>>>> > >>>>> Signed-off-by: Adam Ford <aford173@gmail.com> > >>>> This one breaks Exynos-5433 based TM2e board with a DSI panel. > >>> Marek S, > >>> > >>> Thank you for testing! I knoiw there are several of us who appreciate > >>> your testing this since it's hard to know if something broke without > >>> hardware. Is there any way you can tell me if the flag is set to > >>> enable MIPI_DSI_MODE_VIDEO_BURST? > >> TM2e board uses the DSI panel operated in command mode and handled by > >> panel-samsung-s6e3ha2.c driver. The MIPI_DSI_MODE_VIDEO_BURST flag is > >> not set by the driver. However, the MIPI_DSI_CLOCK_NON_CONTINUOUS flags > >> is set there. I really have no idea if setting VIDEO_BURST would make > >> sense together with CLOCK_NON_CONTINUOUS or not. Maybe the driver lacks > >> setting it? > >> > >> > >>> I was trying to be diligent about not breaking your boards, but > >>> without your boards, it's difficult. The theory was that if > >>> MIPI_DSI_MODE_VIDEO_BURST is set and there is a burst clock set in the > >>> device tree, it would use the burst clock. > >>> > >>> As a fall-back I could just simply check for the presence of the > >>> burst_clock_rate instead of both MIPI_DSI_MODE_VIDEO_BURST and > >>> burst_clock_rate. > >> Maybe you should extend your check also for the > >> MIPI_DSI_CLOCK_NON_CONTINUOUS flag? Does it make sense? > > Looking at some of the devices that might attach in the future, It > > appears that ti-sn65dsi86.c sets this flag. It's a display port > > bridge, so I would expect it to need a variable clock rate similar to > > how the HDMI bridge that I need works. I am concerned that I make the > > burst clock dependent on MIPI_DSI_CLOCK_NON_CONTINUOUS, it might break > > the Display Port bridge. > > > > I think it's better to just check if the samsung,burst-clock-frequency > > is present in the device tree and use it when present. If it's not > > present, then fall back to the pixel clock of the connected device. > > Right, this sounds rational. > > > I looked at a bunch of Exynos parts, and it looks like they all use > > the samsung,burst-clock-frequency device tree setting. Is that true, > > or did I miss one? > > That true. All Exynos based boards with DSI panels use constant DSI > burst frequency defined in the device tree. Thanks for the feedback. I'll try to get another rev of this series pushed later today or Monday. adam > > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index f165483d5044..cea847b8e23c 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -657,11 +657,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) { - unsigned long hs_clk, byte_clk, esc_clk; + unsigned long hs_clk, byte_clk, esc_clk, pix_clk; unsigned long esc_div; u32 reg; + struct drm_display_mode *m = &dsi->mode; + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); + + /* m->clock is in KHz */ + pix_clk = m->clock * 1000; + + /* Use burst_clk_rate for burst mode, otherwise use the pix_clk */ + if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->burst_clk_rate) + hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); + else + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); - hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); if (!hs_clk) { dev_err(dsi->dev, "failed to configure DSI PLL\n"); return -EFAULT; @@ -1800,10 +1810,11 @@ static int samsung_dsim_parse_dt(struct samsung_dsim *dsi) return PTR_ERR(pll_clk); } + /* If it doesn't exist, use pixel clock instead of failing */ ret = samsung_dsim_of_read_u32(node, "samsung,burst-clock-frequency", &dsi->burst_clk_rate); if (ret < 0) - return ret; + dsi->burst_clk_rate = 0; ret = samsung_dsim_of_read_u32(node, "samsung,esc-clock-frequency", &dsi->esc_clk_rate);
The high-speed clock is hard-coded to the burst-clock frequency specified in the device tree. However, when using devices like certain bridge chips without burst mode and varying resolutions and refresh rates, it may be necessary to set the high-speed clock dynamically based on the desired pixel clock for the connected device. This also removes the need to set a clock speed from the device tree for non-burst mode operation, since the pixel clock rate is the rate requested from the attached device like an HDMI bridge chip. This should have no impact for people using burst-mode and setting the burst clock rate is still required for those users. Signed-off-by: Adam Ford <aford173@gmail.com> --- drivers/gpu/drm/bridge/samsung-dsim.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)