From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Martin Kepplinger <martin.kepplinger@puri.sm> Cc: festevam@gmail.com, krzk@kernel.org, devicetree@vger.kernel.org, kernel@pengutronix.de, kernel@puri.sm, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, m.felsch@pengutronix.de, mchehab@kernel.org, phone-devel@vger.kernel.org, robh@kernel.org, shawnguo@kernel.org, slongerbeam@gmail.com Subject: Re: [PATCH v6 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller Date: Fri, 16 Jul 2021 13:47:30 +0300 [thread overview] Message-ID: <YPFjwvjSCuvC1915@pendragon.ideasonboard.com> (raw) In-Reply-To: <e88d99abbdcbd6a1b2c27849f08721e79f237adc.camel@puri.sm> Hi Martin, On Fri, Jul 16, 2021 at 10:47:14AM +0200, Martin Kepplinger wrote: > Am Freitag, dem 16.07.2021 um 00:52 +0300 schrieb Laurent Pinchart: > > On Thu, Jul 15, 2021 at 09:37:24AM +0200, Martin Kepplinger wrote: > > > Am Mittwoch, dem 14.07.2021 um 21:24 +0300 schrieb Laurent Pinchart: > > > > On Wed, Jul 14, 2021 at 01:19:30PM +0200, Martin Kepplinger wrote: > > > > > Add a driver to support the i.MX8MQ MIPI CSI receiver. The hardware side > > > > > is based on > > > > > https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c?h=imx_5.4.70_2.3.0 > > > > > > > > > > It's built as part of VIDEO_IMX7_CSI because that's documented to support > > > > > i.MX8M platforms. This driver adds i.MX8MQ support where currently only the > > > > > i.MX8MM platform has been supported. > > > > > > > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > > > > --- > > > > > drivers/staging/media/imx/Makefile | 1 + > > > > > drivers/staging/media/imx/imx8mq-mipi-csi2.c | 949 +++++++++++++++++++ > > > > > 2 files changed, 950 insertions(+) > > > > > create mode 100644 drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > > > > > > > > diff --git a/drivers/staging/media/imx/Makefile b/drivers/staging/media/imx/Makefile > > > > > index 6ac33275cc97..19c2fc54d424 100644 > > > > > --- a/drivers/staging/media/imx/Makefile > > > > > +++ b/drivers/staging/media/imx/Makefile > > > > > @@ -16,3 +16,4 @@ obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-mipi-csi2.o > > > > [snip] > > > > > > > +static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state) > > > > > +{ > > > > > + u32 width = state->format_mbus[MIPI_CSI2_PAD_SINK].width; > > > > > + u32 height = state->format_mbus[MIPI_CSI2_PAD_SINK].height; > > > > > + s64 link_freq; > > > > > + u32 lane_rate; > > > > > + > > > > > + /* Calculate the line rate from the pixel rate. */ > > > > > + link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler, > > > > > + state->csi2_fmt->width, > > > > > + state->bus.num_data_lanes * 2); > > > > > + if (link_freq < 0) { > > > > > + dev_err(state->dev, "Unable to obtain link frequency: %d\n", > > > > > + (int)link_freq); > > > > > + return link_freq; > > > > > + } > > > > > + > > > > > + lane_rate = link_freq * 2; > > > > > + if (lane_rate < 80000000 || lane_rate > 1500000000) { > > > > > + dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + /* https://community.nxp.com/t5/i-MX-Processors/Explenation-for-HS-SETTLE-parameter-in-MIPI-CSI-D-PHY-registers/m-p/764275/highlight/true#M118744 */ > > > > > + if (lane_rate < 250000000) > > > > > + state->hs_settle = 0xb; > > > > > + else if (lane_rate < 500000000) > > > > > + state->hs_settle = 0x8; > > > > > + else > > > > > + state->hs_settle = 0x6; > > > > > > > > We could possibly compute this value based on the formula from the table > > > > in that page, but maybe that's overkill ? If you want to give it a try, > > > > it would be along those lines. > > > > > > > > /* > > > > * The D-PHY specification requires Ths-settle to be in the range > > > > * 85ns + 6*UI to 140ns + 10*UI, with the unit interval UI being half > > > > * the clock period. > > > > * > > > > * The Ths-settle value is expressed in the hardware as a multiple of > > > > * the Esc clock period: > > > > * > > > > * Ths-settle = (PRG_RXHS_SETTLE + 1) * Tperiod of RxClkInEsc > > > > * > > > > * Due to the one cycle inaccuracy introduced by rounding, the > > > > * documentation recommends picking a value away from the boundaries. > > > > * Let's pick the average. > > > > */ > > > > esc_clk_rate = clk_get_rate(...); > > > > > > > > min_ths_settle = 85 + 6 * 1000000 / (lane_rate / 1000); > > > > max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000); > > > > ths_settle = (min_ths_settle + max_ths_settle) / 2; > > > > > > > > state->hs_settle = ths_settle * esc_clk_rate / 1000000000 - 1; > > > > > > I experimented a bit but would like to leave this as a task for later > > > if that's ok. it's correct and simple now. also, using clks[i].clk > > > based on the name string would feel better to submit seperately > > > later. > > > > That's OK with me, but I may then submit a patch on top fairly soon :-) > > Have you been able to test if this code works on your device ? The main > > reason why I think it's better is that it doesn't hardcode a specific > > escape clock frequency assumption, so it should be able to accommodate a > > wider range of use cases. If we change it later, there's always a risk > > of regressions, while if we do this from the start, we'll figure out > > quickly if it doesn't work in some cases. > > taking your code basically as-is doesn't yet work, but it helps a bit. Thanks for testing. > tbh I don't even know how to correctly read that table / calculation: > what is the exact relation of the calculated Ths_settle time inverval > to the hs_settle register bits? The PRG_RXHS_SETTLE field stores a number of timer ticks to cover the Ths-settle internal. The D-PHY arms the timer when it detects the transition to LP-00, and ignores transitions on the lane until the timer expires. The timer is clocked by the escape clock. What hs_settle value do you currently use, and what value does my code produce ? > if the 2 of us can't quickly figure it out I can ask NXP via that > community forum issue and I created > https://source.puri.sm/Librem5/linux-next/-/issues/340 so I won't > forget about it. -- Regards, Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Martin Kepplinger <martin.kepplinger@puri.sm> Cc: festevam@gmail.com, krzk@kernel.org, devicetree@vger.kernel.org, kernel@pengutronix.de, kernel@puri.sm, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, m.felsch@pengutronix.de, mchehab@kernel.org, phone-devel@vger.kernel.org, robh@kernel.org, shawnguo@kernel.org, slongerbeam@gmail.com Subject: Re: [PATCH v6 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller Date: Fri, 16 Jul 2021 13:47:30 +0300 [thread overview] Message-ID: <YPFjwvjSCuvC1915@pendragon.ideasonboard.com> (raw) In-Reply-To: <e88d99abbdcbd6a1b2c27849f08721e79f237adc.camel@puri.sm> Hi Martin, On Fri, Jul 16, 2021 at 10:47:14AM +0200, Martin Kepplinger wrote: > Am Freitag, dem 16.07.2021 um 00:52 +0300 schrieb Laurent Pinchart: > > On Thu, Jul 15, 2021 at 09:37:24AM +0200, Martin Kepplinger wrote: > > > Am Mittwoch, dem 14.07.2021 um 21:24 +0300 schrieb Laurent Pinchart: > > > > On Wed, Jul 14, 2021 at 01:19:30PM +0200, Martin Kepplinger wrote: > > > > > Add a driver to support the i.MX8MQ MIPI CSI receiver. The hardware side > > > > > is based on > > > > > https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c?h=imx_5.4.70_2.3.0 > > > > > > > > > > It's built as part of VIDEO_IMX7_CSI because that's documented to support > > > > > i.MX8M platforms. This driver adds i.MX8MQ support where currently only the > > > > > i.MX8MM platform has been supported. > > > > > > > > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm> > > > > > --- > > > > > drivers/staging/media/imx/Makefile | 1 + > > > > > drivers/staging/media/imx/imx8mq-mipi-csi2.c | 949 +++++++++++++++++++ > > > > > 2 files changed, 950 insertions(+) > > > > > create mode 100644 drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > > > > > > > > diff --git a/drivers/staging/media/imx/Makefile b/drivers/staging/media/imx/Makefile > > > > > index 6ac33275cc97..19c2fc54d424 100644 > > > > > --- a/drivers/staging/media/imx/Makefile > > > > > +++ b/drivers/staging/media/imx/Makefile > > > > > @@ -16,3 +16,4 @@ obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-mipi-csi2.o > > > > [snip] > > > > > > > +static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state) > > > > > +{ > > > > > + u32 width = state->format_mbus[MIPI_CSI2_PAD_SINK].width; > > > > > + u32 height = state->format_mbus[MIPI_CSI2_PAD_SINK].height; > > > > > + s64 link_freq; > > > > > + u32 lane_rate; > > > > > + > > > > > + /* Calculate the line rate from the pixel rate. */ > > > > > + link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler, > > > > > + state->csi2_fmt->width, > > > > > + state->bus.num_data_lanes * 2); > > > > > + if (link_freq < 0) { > > > > > + dev_err(state->dev, "Unable to obtain link frequency: %d\n", > > > > > + (int)link_freq); > > > > > + return link_freq; > > > > > + } > > > > > + > > > > > + lane_rate = link_freq * 2; > > > > > + if (lane_rate < 80000000 || lane_rate > 1500000000) { > > > > > + dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + /* https://community.nxp.com/t5/i-MX-Processors/Explenation-for-HS-SETTLE-parameter-in-MIPI-CSI-D-PHY-registers/m-p/764275/highlight/true#M118744 */ > > > > > + if (lane_rate < 250000000) > > > > > + state->hs_settle = 0xb; > > > > > + else if (lane_rate < 500000000) > > > > > + state->hs_settle = 0x8; > > > > > + else > > > > > + state->hs_settle = 0x6; > > > > > > > > We could possibly compute this value based on the formula from the table > > > > in that page, but maybe that's overkill ? If you want to give it a try, > > > > it would be along those lines. > > > > > > > > /* > > > > * The D-PHY specification requires Ths-settle to be in the range > > > > * 85ns + 6*UI to 140ns + 10*UI, with the unit interval UI being half > > > > * the clock period. > > > > * > > > > * The Ths-settle value is expressed in the hardware as a multiple of > > > > * the Esc clock period: > > > > * > > > > * Ths-settle = (PRG_RXHS_SETTLE + 1) * Tperiod of RxClkInEsc > > > > * > > > > * Due to the one cycle inaccuracy introduced by rounding, the > > > > * documentation recommends picking a value away from the boundaries. > > > > * Let's pick the average. > > > > */ > > > > esc_clk_rate = clk_get_rate(...); > > > > > > > > min_ths_settle = 85 + 6 * 1000000 / (lane_rate / 1000); > > > > max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000); > > > > ths_settle = (min_ths_settle + max_ths_settle) / 2; > > > > > > > > state->hs_settle = ths_settle * esc_clk_rate / 1000000000 - 1; > > > > > > I experimented a bit but would like to leave this as a task for later > > > if that's ok. it's correct and simple now. also, using clks[i].clk > > > based on the name string would feel better to submit seperately > > > later. > > > > That's OK with me, but I may then submit a patch on top fairly soon :-) > > Have you been able to test if this code works on your device ? The main > > reason why I think it's better is that it doesn't hardcode a specific > > escape clock frequency assumption, so it should be able to accommodate a > > wider range of use cases. If we change it later, there's always a risk > > of regressions, while if we do this from the start, we'll figure out > > quickly if it doesn't work in some cases. > > taking your code basically as-is doesn't yet work, but it helps a bit. Thanks for testing. > tbh I don't even know how to correctly read that table / calculation: > what is the exact relation of the calculated Ths_settle time inverval > to the hs_settle register bits? The PRG_RXHS_SETTLE field stores a number of timer ticks to cover the Ths-settle internal. The D-PHY arms the timer when it detects the transition to LP-00, and ignores transitions on the lane until the timer expires. The timer is clocked by the escape clock. What hs_settle value do you currently use, and what value does my code produce ? > if the 2 of us can't quickly figure it out I can ask NXP via that > community forum issue and I created > https://source.puri.sm/Librem5/linux-next/-/issues/340 so I won't > forget about it. -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-07-16 10:47 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-14 11:19 [PATCH v6 0/3] media: imx: add support for imx8mq MIPI RX Martin Kepplinger 2021-07-14 11:19 ` Martin Kepplinger 2021-07-14 11:19 ` [PATCH v6 1/3] dt-bindings: media: document the nxp,imx8mq-mipi-csi2 receiver phy and controller Martin Kepplinger 2021-07-14 11:19 ` [PATCH v6 1/3] dt-bindings: media: document the nxp, imx8mq-mipi-csi2 " Martin Kepplinger 2021-07-14 15:44 ` [PATCH v6 1/3] dt-bindings: media: document the nxp,imx8mq-mipi-csi2 " Rob Herring 2021-07-14 15:44 ` [PATCH v6 1/3] dt-bindings: media: document the nxp, imx8mq-mipi-csi2 " Rob Herring 2021-07-14 17:53 ` [PATCH v6 1/3] dt-bindings: media: document the nxp,imx8mq-mipi-csi2 " Laurent Pinchart 2021-07-14 17:53 ` Laurent Pinchart 2021-07-14 18:07 ` Martin Kepplinger 2021-07-14 18:07 ` Martin Kepplinger 2021-07-14 18:07 ` Martin Kepplinger 2021-07-14 11:19 ` [PATCH v6 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx " Martin Kepplinger 2021-07-14 11:19 ` Martin Kepplinger 2021-07-14 18:24 ` Laurent Pinchart 2021-07-14 18:24 ` Laurent Pinchart 2021-07-15 6:49 ` Martin Kepplinger 2021-07-15 6:49 ` Martin Kepplinger 2021-07-15 6:49 ` Martin Kepplinger 2021-07-15 21:47 ` Laurent Pinchart 2021-07-15 21:47 ` Laurent Pinchart 2021-07-15 7:37 ` Martin Kepplinger 2021-07-15 7:37 ` Martin Kepplinger 2021-07-15 7:37 ` Martin Kepplinger 2021-07-15 21:52 ` Laurent Pinchart 2021-07-15 21:52 ` Laurent Pinchart 2021-07-16 8:47 ` Martin Kepplinger 2021-07-16 8:47 ` Martin Kepplinger 2021-07-16 8:47 ` Martin Kepplinger 2021-07-16 10:47 ` Laurent Pinchart [this message] 2021-07-16 10:47 ` Laurent Pinchart 2021-07-19 10:46 ` Martin Kepplinger 2021-07-19 10:46 ` Martin Kepplinger 2021-07-19 10:46 ` Martin Kepplinger 2021-07-19 14:20 ` Martin Kepplinger 2021-07-19 14:20 ` Martin Kepplinger 2021-07-19 14:20 ` Martin Kepplinger 2021-07-14 11:19 ` [PATCH v6 3/3] arm64: dts: imx8mq: add mipi csi phy and csi bridge descriptions Martin Kepplinger 2021-07-14 11:19 ` Martin Kepplinger
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YPFjwvjSCuvC1915@pendragon.ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=devicetree@vger.kernel.org \ --cc=festevam@gmail.com \ --cc=kernel@pengutronix.de \ --cc=kernel@puri.sm \ --cc=krzk@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-staging@lists.linux.dev \ --cc=m.felsch@pengutronix.de \ --cc=martin.kepplinger@puri.sm \ --cc=mchehab@kernel.org \ --cc=phone-devel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=shawnguo@kernel.org \ --cc=slongerbeam@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.