From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752547AbdF3QBU (ORCPT ); Fri, 30 Jun 2017 12:01:20 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:43429 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752013AbdF3QBS (ORCPT ); Fri, 30 Jun 2017 12:01:18 -0400 Date: Fri, 30 Jun 2017 18:01:06 +0200 From: Maxime Ripard To: Jonathan Liu Cc: Chen-Yu Tsai , David Airlie , linux-kernel , dri-devel , linux-arm-kernel , linux-sunxi Subject: Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Message-ID: <20170630160106.qtm3hdaoalxqu37c@flea> References: <20170627143652.13075-1-net147@gmail.com> <20170628092024.ejxy6itqj3hx6yew@flea> <20170629155620.4keqi4cumbtvv63u@flea> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="23ap22rl46qesy7k" Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --23ap22rl46qesy7k Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jul 01, 2017 at 12:14:44AM +1000, Jonathan Liu wrote: > Hi Chen-Yu and Maxime, >=20 > On 30 June 2017 at 13:16, Chen-Yu Tsai wrote: > > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu wrote: > >> Hi Maxime, > >> > >> On 30 June 2017 at 01:56, Maxime Ripard > >> wrote: > >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: > >>>> >> + u32 int_status; > >>>> >> + u32 fifo_status; > >>>> >> + /* Read needs empty flag unset, write needs full flag unset= */ > >>>> >> + u32 flag =3D read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : > >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; > >>>> >> + int ret; > >>>> >> + > >>>> >> + /* Wait until error or FIFO ready */ > >>>> >> + ret =3D readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_= STATUS_REG, > >>>> >> + int_status, > >>>> >> + is_err_status(int_status) || > >>>> >> + is_fifo_flag_unset(hdmi, &fifo_sta= tus, flag), > >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE)= * byte_time, > >>>> >> + 100000); > >>>> >> + > >>>> >> + if (is_err_status(int_status)) > >>>> >> + return -EIO; > >>>> >> + if (ret) > >>>> >> + return -ETIMEDOUT; > >>>> > > >>>> > Why not just have > >>>> > ret =3D readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS= _REG, reg, > >>>> > !(reg & flag), 100, 100000); > >>>> > > >>>> > if (ret < 0) > >>>> > if (is_err_status()) > >>>> > return -EIO; > >>>> > return ret; > >>>> > > >>>> > > >>>> > >>>> If I check error status after readl_poll_timeout and there is an err= or > >>>> (e.g. the I2C address does not have a corresponding device connected > >>>> or nothing connected to HDMI port) it will keep checking the fifo > >>>> status even though error bit is set in the int status and then timeo= ut > >>>> after 100 ms. If it checks the int status register at the same time, > >>>> it will error after 100 nanoseconds. I don't want to introduce > >>>> unnecessary delays considering part of the reason for adding this > >>>> driver to make it more usable for non-standard use cases. > >>> > >>> Well, polling for 100ms doesn't seem great either. What was the > >>> rationale behind that timeout? > >>> > >> > >> When an error occurs one of the error bits will be set in the > >> INT_STATUS register so this is detected very quickly if I check the > >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in > >> case the I2C slave does clock stretching in which case the transfer > >> may take longer than the predicted time. > >> > >>> And we can also reverse the check and look at the INT_STATUS > >>> register. The errors will be there, and we can program the threshold > >>> we want in both directions and use the > >>> DDC_FIFO_Request_Interrupt_Status bit. > >>> > >> > >> I did try that when I was doing the v3 patch but I couldn't get it to > >> work as mentioned previously in the v3 patch discussion. I programmed > >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl > >> register at the same time as setting FIFO_Address_Clear but the > >> request interrupt status bit did not get updated to the appropriate > >> state that is consistent with the FIFO level and the thresholds. I did > >> try this several times for subsequent patch versions without success. > > > > The manual says "When FIFO level is above this value in read mode, DMA > > request and FIFO request interrupt are asserted if relative enable is o= n." > > > > Perhaps try enabling the interrupts? But if that were the case, wouldn't > > using interrupts instead of polling be better? > > > > ChenYu > > >=20 > I managed to get the thresholds working so switching to using > interrupts instead of polling will be my next goal. I'd say that we can do it using polling first, and then move to interrupts if needed. And that should definitely be part of a separate patch. This one is already pretty invasive. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --23ap22rl46qesy7k Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZVnXCAAoJEBx+YmzsjxAguScP/25/Dhku/Hd4GUnidg2ZHJkg YJ60lvYZsDl91s8lFZgoJCSrE1a1lCGoOCdCz5AXIYtMU6iEN9FX/++DIU5V1hAr FOjsx9reg2R1uHudb8TMHq6w10TffekMvMt3tgyOUKajP5on2uCN7SCJ8kukZeOZ A78/z85weLzhysH4xTAkyYdq5T3K+I22BDGc8kxIhWPnl2jMAkneFgC3+/psqfmW wYO/TAnF0WkmAeZlGXnpQ87cNB8SxkejEO9uK6gQkdy2HHdKlHFf347GoqhKd3Ns QJIV2Sh5gM52pewu/U+mkeRmBGjBNCMQPAsExoXZU40+o5YoC68XCwYHkF5a01lF LfM4glfVWywV+rqXp9xc6onrke4A7668I+USCkDGnLr9NLN6ydcok8v8aOp0P5as mPheSO755kj7YI25x23wLlLifaMI7eM/ByaIzXCXfGhaN92B5VRIwvt6zbS8hr/5 h/rnpZ6zoO/fDhsHCILTMknnezCoZYEJcNdutMgkkVir9UjzKS5UOnSW/kKfuvn7 drnOF8Z2idLjgMtn5dEuKuBlhO/hUogUK7WqWbiTup4/J/3OvUs28Vyjt3sZvno5 qRkcrO+2z51XE9zmf1OALqENESt1F7k2Q9wf4AO1cHeugXpz4MAD+LQV6vdO442/ 2UcyImLaD9eK0+ZxSLWG =PghK -----END PGP SIGNATURE----- --23ap22rl46qesy7k-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Fri, 30 Jun 2017 18:01:06 +0200 Subject: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus In-Reply-To: References: <20170627143652.13075-1-net147@gmail.com> <20170628092024.ejxy6itqj3hx6yew@flea> <20170629155620.4keqi4cumbtvv63u@flea> Message-ID: <20170630160106.qtm3hdaoalxqu37c@flea> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sat, Jul 01, 2017 at 12:14:44AM +1000, Jonathan Liu wrote: > Hi Chen-Yu and Maxime, > > On 30 June 2017 at 13:16, Chen-Yu Tsai wrote: > > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu wrote: > >> Hi Maxime, > >> > >> On 30 June 2017 at 01:56, Maxime Ripard > >> wrote: > >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: > >>>> >> + u32 int_status; > >>>> >> + u32 fifo_status; > >>>> >> + /* Read needs empty flag unset, write needs full flag unset */ > >>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : > >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; > >>>> >> + int ret; > >>>> >> + > >>>> >> + /* Wait until error or FIFO ready */ > >>>> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG, > >>>> >> + int_status, > >>>> >> + is_err_status(int_status) || > >>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, flag), > >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * byte_time, > >>>> >> + 100000); > >>>> >> + > >>>> >> + if (is_err_status(int_status)) > >>>> >> + return -EIO; > >>>> >> + if (ret) > >>>> >> + return -ETIMEDOUT; > >>>> > > >>>> > Why not just have > >>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg, > >>>> > !(reg & flag), 100, 100000); > >>>> > > >>>> > if (ret < 0) > >>>> > if (is_err_status()) > >>>> > return -EIO; > >>>> > return ret; > >>>> > > >>>> > > >>>> > >>>> If I check error status after readl_poll_timeout and there is an error > >>>> (e.g. the I2C address does not have a corresponding device connected > >>>> or nothing connected to HDMI port) it will keep checking the fifo > >>>> status even though error bit is set in the int status and then timeout > >>>> after 100 ms. If it checks the int status register at the same time, > >>>> it will error after 100 nanoseconds. I don't want to introduce > >>>> unnecessary delays considering part of the reason for adding this > >>>> driver to make it more usable for non-standard use cases. > >>> > >>> Well, polling for 100ms doesn't seem great either. What was the > >>> rationale behind that timeout? > >>> > >> > >> When an error occurs one of the error bits will be set in the > >> INT_STATUS register so this is detected very quickly if I check the > >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in > >> case the I2C slave does clock stretching in which case the transfer > >> may take longer than the predicted time. > >> > >>> And we can also reverse the check and look at the INT_STATUS > >>> register. The errors will be there, and we can program the threshold > >>> we want in both directions and use the > >>> DDC_FIFO_Request_Interrupt_Status bit. > >>> > >> > >> I did try that when I was doing the v3 patch but I couldn't get it to > >> work as mentioned previously in the v3 patch discussion. I programmed > >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl > >> register at the same time as setting FIFO_Address_Clear but the > >> request interrupt status bit did not get updated to the appropriate > >> state that is consistent with the FIFO level and the thresholds. I did > >> try this several times for subsequent patch versions without success. > > > > The manual says "When FIFO level is above this value in read mode, DMA > > request and FIFO request interrupt are asserted if relative enable is on." > > > > Perhaps try enabling the interrupts? But if that were the case, wouldn't > > using interrupts instead of polling be better? > > > > ChenYu > > > > I managed to get the thresholds working so switching to using > interrupts instead of polling will be my next goal. I'd say that we can do it using polling first, and then move to interrupts if needed. And that should definitely be part of a separate patch. This one is already pretty invasive. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus Date: Fri, 30 Jun 2017 18:01:06 +0200 Message-ID: <20170630160106.qtm3hdaoalxqu37c@flea> References: <20170627143652.13075-1-net147@gmail.com> <20170628092024.ejxy6itqj3hx6yew@flea> <20170629155620.4keqi4cumbtvv63u@flea> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0433698281==" Return-path: Received: from mail.free-electrons.com (mail.free-electrons.com [62.4.15.54]) by gabe.freedesktop.org (Postfix) with ESMTP id 2CDE06E831 for ; Fri, 30 Jun 2017 16:01:18 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jonathan Liu Cc: linux-sunxi , dri-devel , linux-kernel , Chen-Yu Tsai , linux-arm-kernel List-Id: dri-devel@lists.freedesktop.org --===============0433698281== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="23ap22rl46qesy7k" Content-Disposition: inline --23ap22rl46qesy7k Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jul 01, 2017 at 12:14:44AM +1000, Jonathan Liu wrote: > Hi Chen-Yu and Maxime, >=20 > On 30 June 2017 at 13:16, Chen-Yu Tsai wrote: > > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu wrote: > >> Hi Maxime, > >> > >> On 30 June 2017 at 01:56, Maxime Ripard > >> wrote: > >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: > >>>> >> + u32 int_status; > >>>> >> + u32 fifo_status; > >>>> >> + /* Read needs empty flag unset, write needs full flag unset= */ > >>>> >> + u32 flag =3D read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : > >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; > >>>> >> + int ret; > >>>> >> + > >>>> >> + /* Wait until error or FIFO ready */ > >>>> >> + ret =3D readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_= STATUS_REG, > >>>> >> + int_status, > >>>> >> + is_err_status(int_status) || > >>>> >> + is_fifo_flag_unset(hdmi, &fifo_sta= tus, flag), > >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE)= * byte_time, > >>>> >> + 100000); > >>>> >> + > >>>> >> + if (is_err_status(int_status)) > >>>> >> + return -EIO; > >>>> >> + if (ret) > >>>> >> + return -ETIMEDOUT; > >>>> > > >>>> > Why not just have > >>>> > ret =3D readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS= _REG, reg, > >>>> > !(reg & flag), 100, 100000); > >>>> > > >>>> > if (ret < 0) > >>>> > if (is_err_status()) > >>>> > return -EIO; > >>>> > return ret; > >>>> > > >>>> > > >>>> > >>>> If I check error status after readl_poll_timeout and there is an err= or > >>>> (e.g. the I2C address does not have a corresponding device connected > >>>> or nothing connected to HDMI port) it will keep checking the fifo > >>>> status even though error bit is set in the int status and then timeo= ut > >>>> after 100 ms. If it checks the int status register at the same time, > >>>> it will error after 100 nanoseconds. I don't want to introduce > >>>> unnecessary delays considering part of the reason for adding this > >>>> driver to make it more usable for non-standard use cases. > >>> > >>> Well, polling for 100ms doesn't seem great either. What was the > >>> rationale behind that timeout? > >>> > >> > >> When an error occurs one of the error bits will be set in the > >> INT_STATUS register so this is detected very quickly if I check the > >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in > >> case the I2C slave does clock stretching in which case the transfer > >> may take longer than the predicted time. > >> > >>> And we can also reverse the check and look at the INT_STATUS > >>> register. The errors will be there, and we can program the threshold > >>> we want in both directions and use the > >>> DDC_FIFO_Request_Interrupt_Status bit. > >>> > >> > >> I did try that when I was doing the v3 patch but I couldn't get it to > >> work as mentioned previously in the v3 patch discussion. I programmed > >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl > >> register at the same time as setting FIFO_Address_Clear but the > >> request interrupt status bit did not get updated to the appropriate > >> state that is consistent with the FIFO level and the thresholds. I did > >> try this several times for subsequent patch versions without success. > > > > The manual says "When FIFO level is above this value in read mode, DMA > > request and FIFO request interrupt are asserted if relative enable is o= n." > > > > Perhaps try enabling the interrupts? But if that were the case, wouldn't > > using interrupts instead of polling be better? > > > > ChenYu > > >=20 > I managed to get the thresholds working so switching to using > interrupts instead of polling will be my next goal. I'd say that we can do it using polling first, and then move to interrupts if needed. And that should definitely be part of a separate patch. This one is already pretty invasive. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --23ap22rl46qesy7k Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZVnXCAAoJEBx+YmzsjxAguScP/25/Dhku/Hd4GUnidg2ZHJkg YJ60lvYZsDl91s8lFZgoJCSrE1a1lCGoOCdCz5AXIYtMU6iEN9FX/++DIU5V1hAr FOjsx9reg2R1uHudb8TMHq6w10TffekMvMt3tgyOUKajP5on2uCN7SCJ8kukZeOZ A78/z85weLzhysH4xTAkyYdq5T3K+I22BDGc8kxIhWPnl2jMAkneFgC3+/psqfmW wYO/TAnF0WkmAeZlGXnpQ87cNB8SxkejEO9uK6gQkdy2HHdKlHFf347GoqhKd3Ns QJIV2Sh5gM52pewu/U+mkeRmBGjBNCMQPAsExoXZU40+o5YoC68XCwYHkF5a01lF LfM4glfVWywV+rqXp9xc6onrke4A7668I+USCkDGnLr9NLN6ydcok8v8aOp0P5as mPheSO755kj7YI25x23wLlLifaMI7eM/ByaIzXCXfGhaN92B5VRIwvt6zbS8hr/5 h/rnpZ6zoO/fDhsHCILTMknnezCoZYEJcNdutMgkkVir9UjzKS5UOnSW/kKfuvn7 drnOF8Z2idLjgMtn5dEuKuBlhO/hUogUK7WqWbiTup4/J/3OvUs28Vyjt3sZvno5 qRkcrO+2z51XE9zmf1OALqENESt1F7k2Q9wf4AO1cHeugXpz4MAD+LQV6vdO442/ 2UcyImLaD9eK0+ZxSLWG =PghK -----END PGP SIGNATURE----- --23ap22rl46qesy7k-- --===============0433698281== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0433698281==--