| Message ID | 20230506195325.876871-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: imx: composite-8m: Add imx8m_divider_determine_rate | expand |
On Thu, May 11, 2023 at 10:03 PM Adam Ford <aford173@gmail.com> wrote: > > On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote: > > > > Currently, certain clocks are derrived as a divider from their > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > > is set, the parent clock is not properly set which can lead > > to some relatively inaccurate clock values. > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > > cannot rely on calling a standard determine_rate function, > > because the 8m composite clocks have a pre-divider and > > post-divider. Because of this, a custom determine_rate > > function is necessary to determine the maximum clock > > division which is equivalent to pre-divider * the > > post-divider. > > > > With this added, the system can attempt to adjust the parent rate > > when the proper flags are set which can lead to a more precise clock > > value. > > > > On the imx8mplus, no clock changes are present. > > On the Mini and Nano, this can help achieve more accurate > > lcdif clocks. When trying to get a pixel clock of 31.500MHz > > on an imx8m Nano, the clocks divided the 594MHz down, but > > left the parent rate untouched which caused a calulation error. > > > > Before: > > video_pll 594000000 > > video_pll_bypass 594000000 > > video_pll_out 594000000 > > disp_pixel 31263158 > > disp_pixel_clk 31263158 > > > > Variance = -236842 Hz > > > > After this patch: > > video_pll 31500000 > > video_pll_bypass 31500000 > > video_pll_out 31500000 > > disp_pixel 31500000 > > disp_pixel_clk 31500000 > > > > Variance = 0 Hz > > > > All other clocks rates and parent were the same. > > Similar results on imx8mm were found. > > > > Peng / Abel, > > I was curious if either of you might have time to review this attempt > at enabling determine_rate on the 8m's. I tested this on the 8mm, > 8mn, and 8mp, and found no regressions. Gentle nudge. It's been several weeks since the initial post and the DSI driver is now available for Mini and Nano, so having this in Mini and Nano will really help it sync various video sources. thanks, adam > > adam > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > V2: Fix build warning found by build bot and fix prediv_value > > and div_value because the values stored are the divisor - 1, > > so we need to add 1 to the values to be correct. > > > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c > > index cbf0d7955a00..7a6e3ce97133 100644 > > --- a/drivers/clk/imx/clk-composite-8m.c > > +++ b/drivers/clk/imx/clk-composite-8m.c > > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > > return ret; > > } > > > > +static int imx8m_divider_determine_rate(struct clk_hw *hw, > > + struct clk_rate_request *req) > > +{ > > + struct clk_divider *divider = to_clk_divider(hw); > > + int prediv_value; > > + int div_value; > > + > > + /* if read only, just return current value */ > > + if (divider->flags & CLK_DIVIDER_READ_ONLY) { > > + u32 val; > > + > > + val = readl(divider->reg); > > + prediv_value = val >> divider->shift; > > + prediv_value &= clk_div_mask(divider->width); > > + prediv_value++; > > + > > + div_value = val >> PCG_DIV_SHIFT; > > + div_value &= clk_div_mask(PCG_DIV_WIDTH); > > + div_value++; > > + > > + return divider_ro_determine_rate(hw, req, divider->table, > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > + divider->flags, prediv_value * div_value); > > + } > > + > > + return divider_determine_rate(hw, req, divider->table, > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > + divider->flags); > > +} > > + > > static const struct clk_ops imx8m_clk_composite_divider_ops = { > > .recalc_rate = imx8m_clk_composite_divider_recalc_rate, > > .round_rate = imx8m_clk_composite_divider_round_rate, > > .set_rate = imx8m_clk_composite_divider_set_rate, > > + .determine_rate = imx8m_divider_determine_rate, > > }; > > > > static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw) > > -- > > 2.39.2 > >
> Subject: Re: [PATCH] clk: imx: composite-8m: Add > imx8m_divider_determine_rate > > On Thu, May 11, 2023 at 10:03 PM Adam Ford <aford173@gmail.com> > wrote: > > > > On Sat, May 6, 2023 at 2:53 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > Currently, certain clocks are derrived as a divider from their > > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT is > > > set, the parent clock is not properly set which can lead to some > > > relatively inaccurate clock values. > > > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it cannot rely > > > on calling a standard determine_rate function, because the 8m > > > composite clocks have a pre-divider and post-divider. Because of > > > this, a custom determine_rate function is necessary to determine the > > > maximum clock division which is equivalent to pre-divider * the > > > post-divider. > > > > > > With this added, the system can attempt to adjust the parent rate > > > when the proper flags are set which can lead to a more precise clock > > > value. > > > > > > On the imx8mplus, no clock changes are present. > > > On the Mini and Nano, this can help achieve more accurate lcdif > > > clocks. When trying to get a pixel clock of 31.500MHz on an imx8m > > > Nano, the clocks divided the 594MHz down, but left the parent rate > > > untouched which caused a calulation error. > > > > > > Before: > > > video_pll 594000000 > > > video_pll_bypass 594000000 > > > video_pll_out 594000000 > > > disp_pixel 31263158 > > > disp_pixel_clk 31263158 > > > > > > Variance = -236842 Hz > > > > > > After this patch: > > > video_pll 31500000 > > > video_pll_bypass 31500000 > > > video_pll_out 31500000 > > > disp_pixel 31500000 > > > disp_pixel_clk 31500000 > > > > > > Variance = 0 Hz > > > > > > All other clocks rates and parent were the same. > > > Similar results on imx8mm were found. > > > > > > > Peng / Abel, > > > > I was curious if either of you might have time to review this attempt > > at enabling determine_rate on the 8m's. I tested this on the 8mm, > > 8mn, and 8mp, and found no regressions. > > Gentle nudge. > > It's been several weeks since the initial post and the DSI driver is now > available for Mini and Nano, so having this in Mini and Nano will really help > it sync various video sources. Sorry, overlooked this patch. Will take a look. Regards, Peng. > > thanks, > > adam > > > > > adam > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to > > > determine_rate"") > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > --- > > > V2: Fix build warning found by build bot and fix prediv_value > > > and div_value because the values stored are the divisor - 1, > > > so we need to add 1 to the values to be correct. > > > > > > diff --git a/drivers/clk/imx/clk-composite-8m.c > > > b/drivers/clk/imx/clk-composite-8m.c > > > index cbf0d7955a00..7a6e3ce97133 100644 > > > --- a/drivers/clk/imx/clk-composite-8m.c > > > +++ b/drivers/clk/imx/clk-composite-8m.c > > > @@ -119,10 +119,41 @@ static int > imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > > > return ret; > > > } > > > > > > +static int imx8m_divider_determine_rate(struct clk_hw *hw, > > > + struct clk_rate_request *req) > > > +{ > > > + struct clk_divider *divider = to_clk_divider(hw); > > > + int prediv_value; > > > + int div_value; > > > + > > > + /* if read only, just return current value */ > > > + if (divider->flags & CLK_DIVIDER_READ_ONLY) { > > > + u32 val; > > > + > > > + val = readl(divider->reg); > > > + prediv_value = val >> divider->shift; > > > + prediv_value &= clk_div_mask(divider->width); > > > + prediv_value++; > > > + > > > + div_value = val >> PCG_DIV_SHIFT; > > > + div_value &= clk_div_mask(PCG_DIV_WIDTH); > > > + div_value++; > > > + > > > + return divider_ro_determine_rate(hw, req, divider->table, > > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > > + divider->flags, prediv_value * div_value); > > > + } > > > + > > > + return divider_determine_rate(hw, req, divider->table, > > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > > + divider->flags); } > > > + > > > static const struct clk_ops imx8m_clk_composite_divider_ops = { > > > .recalc_rate = imx8m_clk_composite_divider_recalc_rate, > > > .round_rate = imx8m_clk_composite_divider_round_rate, > > > .set_rate = imx8m_clk_composite_divider_set_rate, > > > + .determine_rate = imx8m_divider_determine_rate, > > > }; > > > > > > static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw) > > > -- > > > 2.39.2 > > >
On 5/7/2023 3:53 AM, Adam Ford wrote: > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > Currently, certain clocks are derrived as a divider from their > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > is set, the parent clock is not properly set which can lead > to some relatively inaccurate clock values. > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > cannot rely on calling a standard determine_rate function, > because the 8m composite clocks have a pre-divider and > post-divider. Because of this, a custom determine_rate > function is necessary to determine the maximum clock > division which is equivalent to pre-divider * the > post-divider. > > With this added, the system can attempt to adjust the parent rate > when the proper flags are set which can lead to a more precise clock > value. > > On the imx8mplus, no clock changes are present. > On the Mini and Nano, this can help achieve more accurate > lcdif clocks. When trying to get a pixel clock of 31.500MHz > on an imx8m Nano, the clocks divided the 594MHz down, but > left the parent rate untouched which caused a calulation error. Not all clocks has pre/post div both. If CLK_SET_RATE_PARENT not set, would there be any issues for other clocks? Regards, Peng. > > Before: > video_pll 594000000 > video_pll_bypass 594000000 > video_pll_out 594000000 > disp_pixel 31263158 > disp_pixel_clk 31263158 > > Variance = -236842 Hz > > After this patch: > video_pll 31500000 > video_pll_bypass 31500000 > video_pll_out 31500000 > disp_pixel 31500000 > disp_pixel_clk 31500000 > > Variance = 0 Hz > > All other clocks rates and parent were the same. > Similar results on imx8mm were found. > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > V2: Fix build warning found by build bot and fix prediv_value > and div_value because the values stored are the divisor - 1, > so we need to add 1 to the values to be correct. > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c > index cbf0d7955a00..7a6e3ce97133 100644 > --- a/drivers/clk/imx/clk-composite-8m.c > +++ b/drivers/clk/imx/clk-composite-8m.c > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > return ret; > } > > +static int imx8m_divider_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > +{ > + struct clk_divider *divider = to_clk_divider(hw); > + int prediv_value; > + int div_value; > + > + /* if read only, just return current value */ > + if (divider->flags & CLK_DIVIDER_READ_ONLY) { > + u32 val; > + > + val = readl(divider->reg); > + prediv_value = val >> divider->shift; > + prediv_value &= clk_div_mask(divider->width); > + prediv_value++; > + > + div_value = val >> PCG_DIV_SHIFT; > + div_value &= clk_div_mask(PCG_DIV_WIDTH); > + div_value++; > + > + return divider_ro_determine_rate(hw, req, divider->table, > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > + divider->flags, prediv_value * div_value); > + } > + > + return divider_determine_rate(hw, req, divider->table, > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > + divider->flags); > +} > + > static const struct clk_ops imx8m_clk_composite_divider_ops = { > .recalc_rate = imx8m_clk_composite_divider_recalc_rate, > .round_rate = imx8m_clk_composite_divider_round_rate, > .set_rate = imx8m_clk_composite_divider_set_rate, > + .determine_rate = imx8m_divider_determine_rate, > }; > > static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw) > -- > 2.39.2 >
On Mon, May 22, 2023 at 9:33 PM Peng Fan <peng.fan@oss.nxp.com> wrote: > > > > On 5/7/2023 3:53 AM, Adam Ford wrote: > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > > > > Currently, certain clocks are derrived as a divider from their > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > > is set, the parent clock is not properly set which can lead > > to some relatively inaccurate clock values. > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > > cannot rely on calling a standard determine_rate function, > > because the 8m composite clocks have a pre-divider and > > post-divider. Because of this, a custom determine_rate > > function is necessary to determine the maximum clock > > division which is equivalent to pre-divider * the > > post-divider. > > > > With this added, the system can attempt to adjust the parent rate > > when the proper flags are set which can lead to a more precise clock > > value. > > > > On the imx8mplus, no clock changes are present. > > On the Mini and Nano, this can help achieve more accurate > > lcdif clocks. When trying to get a pixel clock of 31.500MHz > > on an imx8m Nano, the clocks divided the 594MHz down, but > > left the parent rate untouched which caused a calulation error. > > Not all clocks has pre/post div both. > > If CLK_SET_RATE_PARENT not set, would there be any issues for > other clocks? I did a dump of the clk_summary for Mini, Nano and Plus, and I found no changes to any clock other than the video_pll, and most of the clocks do not have CLK_SET_RATE_PARENT set, so from what I could tell it seemed harmless. > > Regards, > Peng. > > > > > Before: > > video_pll 594000000 > > video_pll_bypass 594000000 > > video_pll_out 594000000 > > disp_pixel 31263158 > > disp_pixel_clk 31263158 > > > > Variance = -236842 Hz > > > > After this patch: > > video_pll 31500000 > > video_pll_bypass 31500000 > > video_pll_out 31500000 > > disp_pixel 31500000 > > disp_pixel_clk 31500000 > > > > Variance = 0 Hz > > > > All other clocks rates and parent were the same. > > Similar results on imx8mm were found. > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > V2: Fix build warning found by build bot and fix prediv_value > > and div_value because the values stored are the divisor - 1, > > so we need to add 1 to the values to be correct. > > > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c > > index cbf0d7955a00..7a6e3ce97133 100644 > > --- a/drivers/clk/imx/clk-composite-8m.c > > +++ b/drivers/clk/imx/clk-composite-8m.c > > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > > return ret; > > } > > > > +static int imx8m_divider_determine_rate(struct clk_hw *hw, > > + struct clk_rate_request *req) > > +{ > > + struct clk_divider *divider = to_clk_divider(hw); > > + int prediv_value; > > + int div_value; > > + > > + /* if read only, just return current value */ > > + if (divider->flags & CLK_DIVIDER_READ_ONLY) { > > + u32 val; > > + > > + val = readl(divider->reg); > > + prediv_value = val >> divider->shift; > > + prediv_value &= clk_div_mask(divider->width); > > + prediv_value++; > > + > > + div_value = val >> PCG_DIV_SHIFT; > > + div_value &= clk_div_mask(PCG_DIV_WIDTH); > > + div_value++; > > + > > + return divider_ro_determine_rate(hw, req, divider->table, > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > + divider->flags, prediv_value * div_value); > > + } > > + > > + return divider_determine_rate(hw, req, divider->table, > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > + divider->flags); > > +} > > + > > static const struct clk_ops imx8m_clk_composite_divider_ops = { > > .recalc_rate = imx8m_clk_composite_divider_recalc_rate, > > .round_rate = imx8m_clk_composite_divider_round_rate, > > .set_rate = imx8m_clk_composite_divider_set_rate, > > + .determine_rate = imx8m_divider_determine_rate, > > }; > > > > static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw) > > -- > > 2.39.2 > >
On Mon, May 22, 2023 at 10:23 PM Adam Ford <aford173@gmail.com> wrote: > > On Mon, May 22, 2023 at 9:33 PM Peng Fan <peng.fan@oss.nxp.com> wrote: > > > > > > > > On 5/7/2023 3:53 AM, Adam Ford wrote: > > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > > > > > > > Currently, certain clocks are derrived as a divider from their > > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > > > is set, the parent clock is not properly set which can lead > > > to some relatively inaccurate clock values. > > > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > > > cannot rely on calling a standard determine_rate function, > > > because the 8m composite clocks have a pre-divider and > > > post-divider. Because of this, a custom determine_rate > > > function is necessary to determine the maximum clock > > > division which is equivalent to pre-divider * the > > > post-divider. > > > > > > With this added, the system can attempt to adjust the parent rate > > > when the proper flags are set which can lead to a more precise clock > > > value. > > > > > > On the imx8mplus, no clock changes are present. > > > On the Mini and Nano, this can help achieve more accurate > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz > > > on an imx8m Nano, the clocks divided the 594MHz down, but > > > left the parent rate untouched which caused a calulation error. > > > > Not all clocks has pre/post div both. Peng, Any suggestions on how to identify this? looking at the code that sets the clock rates, it seemed to me like the code was mostly common code and it looked like the pre and post div's were basically the same. CAn you point me to an example where they're identified with different values? > > > > If CLK_SET_RATE_PARENT not set, would there be any issues for > > other clocks? > > I did a dump of the clk_summary for Mini, Nano and Plus, and I found > no changes to any clock other than the video_pll, and most of the > clocks do not have CLK_SET_RATE_PARENT set, so from what I could tell > it seemed harmless. What do you need from me to be able to move this forward? thanks adam > > > > > Regards, > > Peng. > > > > > > > > Before: > > > video_pll 594000000 > > > video_pll_bypass 594000000 > > > video_pll_out 594000000 > > > disp_pixel 31263158 > > > disp_pixel_clk 31263158 > > > > > > Variance = -236842 Hz > > > > > > After this patch: > > > video_pll 31500000 > > > video_pll_bypass 31500000 > > > video_pll_out 31500000 > > > disp_pixel 31500000 > > > disp_pixel_clk 31500000 > > > > > > Variance = 0 Hz > > > > > > All other clocks rates and parent were the same. > > > Similar results on imx8mm were found. > > > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > --- > > > V2: Fix build warning found by build bot and fix prediv_value > > > and div_value because the values stored are the divisor - 1, > > > so we need to add 1 to the values to be correct. > > > > > > diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c > > > index cbf0d7955a00..7a6e3ce97133 100644 > > > --- a/drivers/clk/imx/clk-composite-8m.c > > > +++ b/drivers/clk/imx/clk-composite-8m.c > > > @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, > > > return ret; > > > } > > > > > > +static int imx8m_divider_determine_rate(struct clk_hw *hw, > > > + struct clk_rate_request *req) > > > +{ > > > + struct clk_divider *divider = to_clk_divider(hw); > > > + int prediv_value; > > > + int div_value; > > > + > > > + /* if read only, just return current value */ > > > + if (divider->flags & CLK_DIVIDER_READ_ONLY) { > > > + u32 val; > > > + > > > + val = readl(divider->reg); > > > + prediv_value = val >> divider->shift; > > > + prediv_value &= clk_div_mask(divider->width); > > > + prediv_value++; > > > + > > > + div_value = val >> PCG_DIV_SHIFT; > > > + div_value &= clk_div_mask(PCG_DIV_WIDTH); > > > + div_value++; > > > + > > > + return divider_ro_determine_rate(hw, req, divider->table, > > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > > + divider->flags, prediv_value * div_value); > > > + } > > > + > > > + return divider_determine_rate(hw, req, divider->table, > > > + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, > > > + divider->flags); > > > +} > > > + > > > static const struct clk_ops imx8m_clk_composite_divider_ops = { > > > .recalc_rate = imx8m_clk_composite_divider_recalc_rate, > > > .round_rate = imx8m_clk_composite_divider_round_rate, > > > .set_rate = imx8m_clk_composite_divider_set_rate, > > > + .determine_rate = imx8m_divider_determine_rate, > > > }; > > > > > > static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw) > > > -- > > > 2.39.2 > > >
On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote: > > Currently, certain clocks are derrived as a divider from their > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > is set, the parent clock is not properly set which can lead > to some relatively inaccurate clock values. > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > cannot rely on calling a standard determine_rate function, > because the 8m composite clocks have a pre-divider and > post-divider. Because of this, a custom determine_rate > function is necessary to determine the maximum clock > division which is equivalent to pre-divider * the > post-divider. > > With this added, the system can attempt to adjust the parent rate > when the proper flags are set which can lead to a more precise clock > value. > > On the imx8mplus, no clock changes are present. > On the Mini and Nano, this can help achieve more accurate > lcdif clocks. When trying to get a pixel clock of 31.500MHz > on an imx8m Nano, the clocks divided the 594MHz down, but > left the parent rate untouched which caused a calulation error. > > Before: > video_pll 594000000 > video_pll_bypass 594000000 > video_pll_out 594000000 > disp_pixel 31263158 > disp_pixel_clk 31263158 > > Variance = -236842 Hz > > After this patch: > video_pll 31500000 > video_pll_bypass 31500000 > video_pll_out 31500000 > disp_pixel 31500000 > disp_pixel_clk 31500000 > > Variance = 0 Hz > > All other clocks rates and parent were the same. > Similar results on imx8mm were found. > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > Signed-off-by: Adam Ford <aford173@gmail.com> This works fine on my imx8mm-evk, so: Tested-by: Fabio Estevam <festevam@gmail.com>
On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote: > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > Currently, certain clocks are derrived as a divider from their > > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > > > is set, the parent clock is not properly set which can lead > > > to some relatively inaccurate clock values. > > > > > + Maxime > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > > > cannot rely on calling a standard determine_rate function, > > > because the 8m composite clocks have a pre-divider and > > > post-divider. Because of this, a custom determine_rate > > > function is necessary to determine the maximum clock > > > division which is equivalent to pre-divider * the > > > post-divider. > > > > > > With this added, the system can attempt to adjust the parent rate > > > when the proper flags are set which can lead to a more precise clock > > > value. > > > > > > On the imx8mplus, no clock changes are present. > > > On the Mini and Nano, this can help achieve more accurate > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz > > > on an imx8m Nano, the clocks divided the 594MHz down, but > > > left the parent rate untouched which caused a calulation error. > > > > > > Before: > > > video_pll 594000000 > > > video_pll_bypass 594000000 > > > video_pll_out 594000000 > > > disp_pixel 31263158 > > > disp_pixel_clk 31263158 > > > > > > Variance = -236842 Hz > > > > > > After this patch: > > > video_pll 31500000 > > > video_pll_bypass 31500000 > > > video_pll_out 31500000 > > > disp_pixel 31500000 > > > disp_pixel_clk 31500000 > > > > > > Variance = 0 Hz > > > > > > All other clocks rates and parent were the same. > > > Similar results on imx8mm were found. > > > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > Peng / Abel, > > Any suggestions on how we can move this forward? Looking at the > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the > max values which is basically what my patch does. There was some > discussion about making determine_rate mandatory for muxes[1] and this > patch should help with this in addition to making it easier to sync > more video resolutions on the 8m Mini and Nano. Those patches have been queued by Stephen for 6.6 :) Maxime
On Mon, Jun 12, 2023 at 11:08 AM Maxime Ripard <maxime@cerno.tech> wrote: > > On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote: > > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > Currently, certain clocks are derrived as a divider from their > > > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > > > > is set, the parent clock is not properly set which can lead > > > > to some relatively inaccurate clock values. > > > > > > > > + Maxime > > > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > > > > cannot rely on calling a standard determine_rate function, > > > > because the 8m composite clocks have a pre-divider and > > > > post-divider. Because of this, a custom determine_rate > > > > function is necessary to determine the maximum clock > > > > division which is equivalent to pre-divider * the > > > > post-divider. > > > > > > > > With this added, the system can attempt to adjust the parent rate > > > > when the proper flags are set which can lead to a more precise clock > > > > value. > > > > > > > > On the imx8mplus, no clock changes are present. > > > > On the Mini and Nano, this can help achieve more accurate > > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz > > > > on an imx8m Nano, the clocks divided the 594MHz down, but > > > > left the parent rate untouched which caused a calulation error. > > > > > > > > Before: > > > > video_pll 594000000 > > > > video_pll_bypass 594000000 > > > > video_pll_out 594000000 > > > > disp_pixel 31263158 > > > > disp_pixel_clk 31263158 > > > > > > > > Variance = -236842 Hz > > > > > > > > After this patch: > > > > video_pll 31500000 > > > > video_pll_bypass 31500000 > > > > video_pll_out 31500000 > > > > disp_pixel 31500000 > > > > disp_pixel_clk 31500000 > > > > > > > > Variance = 0 Hz > > > > > > > > All other clocks rates and parent were the same. > > > > Similar results on imx8mm were found. > > > > > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > > Peng / Abel, > > > > Any suggestions on how we can move this forward? Looking at the > > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the > > max values which is basically what my patch does. There was some > > discussion about making determine_rate mandatory for muxes[1] and this > > patch should help with this in addition to making it easier to sync > > more video resolutions on the 8m Mini and Nano. > > Those patches have been queued by Stephen for 6.6 :) One of the patches in the older series was reverted, but this was to address the patch that was reverted. adam > > Maxime
On Mon, Jun 12, 2023 at 11:11:21AM -0500, Adam Ford wrote: > On Mon, Jun 12, 2023 at 11:08 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Sun, Jun 11, 2023 at 12:02:42PM -0500, Adam Ford wrote: > > > On Tue, Jun 6, 2023 at 1:45 PM Fabio Estevam <festevam@gmail.com> wrote: > > > > > > > > On Sat, May 6, 2023 at 4:53 PM Adam Ford <aford173@gmail.com> wrote: > > > > > > > > > > Currently, certain clocks are derrived as a divider from their > > > > > parent clock. For some clocks, even when CLK_SET_RATE_PARENT > > > > > is set, the parent clock is not properly set which can lead > > > > > to some relatively inaccurate clock values. > > > > > > > > > > > + Maxime > > > > > > > > Unlike imx/clk-composite-93 and imx/clk-divider-gate, it > > > > > cannot rely on calling a standard determine_rate function, > > > > > because the 8m composite clocks have a pre-divider and > > > > > post-divider. Because of this, a custom determine_rate > > > > > function is necessary to determine the maximum clock > > > > > division which is equivalent to pre-divider * the > > > > > post-divider. > > > > > > > > > > With this added, the system can attempt to adjust the parent rate > > > > > when the proper flags are set which can lead to a more precise clock > > > > > value. > > > > > > > > > > On the imx8mplus, no clock changes are present. > > > > > On the Mini and Nano, this can help achieve more accurate > > > > > lcdif clocks. When trying to get a pixel clock of 31.500MHz > > > > > on an imx8m Nano, the clocks divided the 594MHz down, but > > > > > left the parent rate untouched which caused a calulation error. > > > > > > > > > > Before: > > > > > video_pll 594000000 > > > > > video_pll_bypass 594000000 > > > > > video_pll_out 594000000 > > > > > disp_pixel 31263158 > > > > > disp_pixel_clk 31263158 > > > > > > > > > > Variance = -236842 Hz > > > > > > > > > > After this patch: > > > > > video_pll 31500000 > > > > > video_pll_bypass 31500000 > > > > > video_pll_out 31500000 > > > > > disp_pixel 31500000 > > > > > disp_pixel_clk 31500000 > > > > > > > > > > Variance = 0 Hz > > > > > > > > > > All other clocks rates and parent were the same. > > > > > Similar results on imx8mm were found. > > > > > > > > > > Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > > > > > Peng / Abel, > > > > > > Any suggestions on how we can move this forward? Looking at the > > > clk-composite-8m driver, imx8m_clk_composite_compute_dividers uses the > > > max values which is basically what my patch does. There was some > > > discussion about making determine_rate mandatory for muxes[1] and this > > > patch should help with this in addition to making it easier to sync > > > more video resolutions on the 8m Mini and Nano. > > > > Those patches have been queued by Stephen for 6.6 :) > > One of the patches in the older series was reverted, but this was to > address the patch that was reverted. Was it? I haven't been cc'd and it doesn't seem to be in -next either? Maxime
diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c index cbf0d7955a00..7a6e3ce97133 100644 --- a/drivers/clk/imx/clk-composite-8m.c +++ b/drivers/clk/imx/clk-composite-8m.c @@ -119,10 +119,41 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw, return ret; } +static int imx8m_divider_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + struct clk_divider *divider = to_clk_divider(hw); + int prediv_value; + int div_value; + + /* if read only, just return current value */ + if (divider->flags & CLK_DIVIDER_READ_ONLY) { + u32 val; + + val = readl(divider->reg); + prediv_value = val >> divider->shift; + prediv_value &= clk_div_mask(divider->width); + prediv_value++; + + div_value = val >> PCG_DIV_SHIFT; + div_value &= clk_div_mask(PCG_DIV_WIDTH); + div_value++; + + return divider_ro_determine_rate(hw, req, divider->table, + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, + divider->flags, prediv_value * div_value); + } + + return divider_determine_rate(hw, req, divider->table, + PCG_PREDIV_WIDTH + PCG_DIV_WIDTH, + divider->flags); +} + static const struct clk_ops imx8m_clk_composite_divider_ops = { .recalc_rate = imx8m_clk_composite_divider_recalc_rate, .round_rate = imx8m_clk_composite_divider_round_rate, .set_rate = imx8m_clk_composite_divider_set_rate, + .determine_rate = imx8m_divider_determine_rate, }; static u8 imx8m_clk_composite_mux_get_parent(struct clk_hw *hw)
Currently, certain clocks are derrived as a divider from their parent clock. For some clocks, even when CLK_SET_RATE_PARENT is set, the parent clock is not properly set which can lead to some relatively inaccurate clock values. Unlike imx/clk-composite-93 and imx/clk-divider-gate, it cannot rely on calling a standard determine_rate function, because the 8m composite clocks have a pre-divider and post-divider. Because of this, a custom determine_rate function is necessary to determine the maximum clock division which is equivalent to pre-divider * the post-divider. With this added, the system can attempt to adjust the parent rate when the proper flags are set which can lead to a more precise clock value. On the imx8mplus, no clock changes are present. On the Mini and Nano, this can help achieve more accurate lcdif clocks. When trying to get a pixel clock of 31.500MHz on an imx8m Nano, the clocks divided the 594MHz down, but left the parent rate untouched which caused a calulation error. Before: video_pll 594000000 video_pll_bypass 594000000 video_pll_out 594000000 disp_pixel 31263158 disp_pixel_clk 31263158 Variance = -236842 Hz After this patch: video_pll 31500000 video_pll_bypass 31500000 video_pll_out 31500000 disp_pixel 31500000 disp_pixel_clk 31500000 Variance = 0 Hz All other clocks rates and parent were the same. Similar results on imx8mm were found. Fixes: 690dccc4a0bf ("Revert "clk: imx: composite-8m: Add support to determine_rate"") Signed-off-by: Adam Ford <aford173@gmail.com> --- V2: Fix build warning found by build bot and fix prediv_value and div_value because the values stored are the divisor - 1, so we need to add 1 to the values to be correct.