From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751585AbeBWHfM (ORCPT ); Fri, 23 Feb 2018 02:35:12 -0500 Received: from mga07.intel.com ([134.134.136.100]:36546 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751541AbeBWHe7 (ORCPT ); Fri, 23 Feb 2018 02:34:59 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,382,1515484800"; d="scan'208";a="206348808" Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for ZynqMP Platform To: Manish Narani , "michal.simek@xilinx.com" , "ulf.hansson@linaro.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "mark.rutland@arm.com" , "robh+dt@kernel.org" Cc: Anirudha Sarangi , Srinivas Goud References: <1517336073-18142-1-git-send-email-mnarani@xilinx.com> <608309c3-fc4d-25a2-f0a9-21c0f3e2db46@intel.com> From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki Message-ID: Date: Fri, 23 Feb 2018 09:34:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23/02/18 09:29, Manish Narani wrote: > > >> -----Original Message----- >> From: Adrian Hunter [mailto:adrian.hunter@intel.com] >> Sent: Thursday, February 22, 2018 1:50 PM >> To: Manish Narani ; michal.simek@xilinx.com; >> ulf.hansson@linaro.org; linux-arm-kernel@lists.infradead.org; linux- >> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org; mark.rutland@arm.com; robh+dt@kernel.org >> Cc: Anirudha Sarangi ; Srinivas Goud >> >> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for >> ZynqMP Platform >> >> On 21/02/18 17:00, Manish Narani wrote: >>> Hi Adrian, >>> >>>> -----Original Message----- >>>> From: Manish Narani >>>> Sent: Wednesday, February 21, 2018 11:39 AM >>>> To: Adrian Hunter ; michal.simek@xilinx.com; >>>> ulf.hansson@linaro.org; linux-arm-kernel@lists.infradead.org; linux- >>>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> devicetree@vger.kernel.org; mark.rutland@arm.com; >> robh+dt@kernel.org >>>> Cc: Anirudha Sarangi ; Srinivas Goud >>>> >>>> Subject: RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning >>>> support for ZynqMP Platform >>>> >>>> Hi Adrian, >>>> >>>> >>>>> -----Original Message----- >>>>> From: Adrian Hunter [mailto:adrian.hunter@intel.com] >>>>> Sent: Friday, February 16, 2018 7:37 PM >>>>> To: Manish Narani ; michal.simek@xilinx.com; >>>>> ulf.hansson@linaro.org; linux-arm-kernel@lists.infradead.org; linux- >>>>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; >>>>> devicetree@vger.kernel.org; mark.rutland@arm.com; >> robh+dt@kernel.org >>>>> Cc: Anirudha Sarangi ; Srinivas Goud >>>>> ; Manish Narani >>>>> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning >>>>> support for ZynqMP Platform >>>>> >>>>> On 30/01/18 20:14, Manish Narani wrote: >>>>>> This patch adds support of SD auto tuning for ZynqMP platform. Auto >>>>>> tuning sequence sends tuning block to card when operating in UHS-1 >>>>>> modes. This resets the DLL and sends CMD19/CMD21 as a part of the >>>>>> auto tuning process. Once the auto tuning process gets completed, >>>>>> reset the DLL to load the newly obtained SDHC tuned tap value. >>>>> >>>>> How is this different from: >>>>> 1. reset the dll >>>>> 2. call sdhci_execute_tuning >>>>> 3. reset the dll >>>>> >>> Below is my take on your above comments: >>> - 'Reset the dll' is a platform specific call inside >>> 'arasan_zynqmp_execute_tuning' which is implemented in >>> sdhci-of-arasan.c >>> - 'arasan_zynqmp_execute_tuning' is called from 'sdhci_execute_tuning' >>> as a platform_execute_tuning routine >>> - So to keep 'DLL reset' routine called from sdhci-of-arasan.c, I have >>> implemented the execute_tuning in sdhci-of-arasan.c >> >> I meant something like: >> >> if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp- >> 8.9a")) >> host->mmc_host_ops.execute_tuning = >> arasan_zynqmp_execute_tuning; >> > This will need the removal of 'const' from 'static const struct mmc_host_ops sdhci_ops = {}' in sdhci.c file. Please confirm. No, it is not const. You are not looking at the right place. i.e. $ grep mmc_host_ops drivers/mmc/host/sdhci.h struct mmc_host_ops mmc_host_ops; /* MMC host ops */ > > Thanks, > Manish >> >> static int arasan_zynqmp_execute_tuning(struct mmc_host *mmc, u32 >> opcode) { >> struct sdhci_host *host = mmc_priv(mmc); >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> struct sdhci_arasan_data *sdhci_arasan = >> sdhci_pltfm_priv(pltfm_host); >> int err; >> >> arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); >> >> err = sdhci_execute_tuning(mmc, opcode); >> if (err) >> return err; >> >> arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); >> >> return 0; >> } >> >>> >>> Alternative way (Please review): >>> - Define a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) in >>> sdhci-of-arasan.c indicating DLL reset needed while tuning operation >>> - Call 'dll reset' routine before and after __sdhci_execute_tuning() >>> in sdhci.c when a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) is >>> set >> >> We should try to avoid quirks. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrian Hunter Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for ZynqMP Platform Date: Fri, 23 Feb 2018 09:34:11 +0200 Message-ID: References: <1517336073-18142-1-git-send-email-mnarani@xilinx.com> <608309c3-fc4d-25a2-f0a9-21c0f3e2db46@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Manish Narani , "michal.simek@xilinx.com" , "ulf.hansson@linaro.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "mark.rutland@arm.com" , "robh+dt@kernel.org" Cc: Srinivas Goud , Anirudha Sarangi List-Id: devicetree@vger.kernel.org On 23/02/18 09:29, Manish Narani wrote: > > >> -----Original Message----- >> From: Adrian Hunter [mailto:adrian.hunter@intel.com] >> Sent: Thursday, February 22, 2018 1:50 PM >> To: Manish Narani ; michal.simek@xilinx.com; >> ulf.hansson@linaro.org; linux-arm-kernel@lists.infradead.org; linux- >> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org; mark.rutland@arm.com; robh+dt@kernel.org >> Cc: Anirudha Sarangi ; Srinivas Goud >> >> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for >> ZynqMP Platform >> >> On 21/02/18 17:00, Manish Narani wrote: >>> Hi Adrian, >>> >>>> -----Original Message----- >>>> From: Manish Narani >>>> Sent: Wednesday, February 21, 2018 11:39 AM >>>> To: Adrian Hunter ; michal.simek@xilinx.com; >>>> ulf.hansson@linaro.org; linux-arm-kernel@lists.infradead.org; linux- >>>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> devicetree@vger.kernel.org; mark.rutland@arm.com; >> robh+dt@kernel.org >>>> Cc: Anirudha Sarangi ; Srinivas Goud >>>> >>>> Subject: RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning >>>> support for ZynqMP Platform >>>> >>>> Hi Adrian, >>>> >>>> >>>>> -----Original Message----- >>>>> From: Adrian Hunter [mailto:adrian.hunter@intel.com] >>>>> Sent: Friday, February 16, 2018 7:37 PM >>>>> To: Manish Narani ; michal.simek@xilinx.com; >>>>> ulf.hansson@linaro.org; linux-arm-kernel@lists.infradead.org; linux- >>>>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; >>>>> devicetree@vger.kernel.org; mark.rutland@arm.com; >> robh+dt@kernel.org >>>>> Cc: Anirudha Sarangi ; Srinivas Goud >>>>> ; Manish Narani >>>>> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning >>>>> support for ZynqMP Platform >>>>> >>>>> On 30/01/18 20:14, Manish Narani wrote: >>>>>> This patch adds support of SD auto tuning for ZynqMP platform. Auto >>>>>> tuning sequence sends tuning block to card when operating in UHS-1 >>>>>> modes. This resets the DLL and sends CMD19/CMD21 as a part of the >>>>>> auto tuning process. Once the auto tuning process gets completed, >>>>>> reset the DLL to load the newly obtained SDHC tuned tap value. >>>>> >>>>> How is this different from: >>>>> 1. reset the dll >>>>> 2. call sdhci_execute_tuning >>>>> 3. reset the dll >>>>> >>> Below is my take on your above comments: >>> - 'Reset the dll' is a platform specific call inside >>> 'arasan_zynqmp_execute_tuning' which is implemented in >>> sdhci-of-arasan.c >>> - 'arasan_zynqmp_execute_tuning' is called from 'sdhci_execute_tuning' >>> as a platform_execute_tuning routine >>> - So to keep 'DLL reset' routine called from sdhci-of-arasan.c, I have >>> implemented the execute_tuning in sdhci-of-arasan.c >> >> I meant something like: >> >> if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp- >> 8.9a")) >> host->mmc_host_ops.execute_tuning = >> arasan_zynqmp_execute_tuning; >> > This will need the removal of 'const' from 'static const struct mmc_host_ops sdhci_ops = {}' in sdhci.c file. Please confirm. No, it is not const. You are not looking at the right place. i.e. $ grep mmc_host_ops drivers/mmc/host/sdhci.h struct mmc_host_ops mmc_host_ops; /* MMC host ops */ > > Thanks, > Manish >> >> static int arasan_zynqmp_execute_tuning(struct mmc_host *mmc, u32 >> opcode) { >> struct sdhci_host *host = mmc_priv(mmc); >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> struct sdhci_arasan_data *sdhci_arasan = >> sdhci_pltfm_priv(pltfm_host); >> int err; >> >> arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); >> >> err = sdhci_execute_tuning(mmc, opcode); >> if (err) >> return err; >> >> arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); >> >> return 0; >> } >> >>> >>> Alternative way (Please review): >>> - Define a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) in >>> sdhci-of-arasan.c indicating DLL reset needed while tuning operation >>> - Call 'dll reset' routine before and after __sdhci_execute_tuning() >>> in sdhci.c when a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) is >>> set >> >> We should try to avoid quirks. From mboxrd@z Thu Jan 1 00:00:00 1970 From: adrian.hunter@intel.com (Adrian Hunter) Date: Fri, 23 Feb 2018 09:34:11 +0200 Subject: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for ZynqMP Platform In-Reply-To: References: <1517336073-18142-1-git-send-email-mnarani@xilinx.com> <608309c3-fc4d-25a2-f0a9-21c0f3e2db46@intel.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 23/02/18 09:29, Manish Narani wrote: > > >> -----Original Message----- >> From: Adrian Hunter [mailto:adrian.hunter at intel.com] >> Sent: Thursday, February 22, 2018 1:50 PM >> To: Manish Narani ; michal.simek at xilinx.com; >> ulf.hansson at linaro.org; linux-arm-kernel at lists.infradead.org; linux- >> mmc at vger.kernel.org; linux-kernel at vger.kernel.org; >> devicetree at vger.kernel.org; mark.rutland at arm.com; robh+dt at kernel.org >> Cc: Anirudha Sarangi ; Srinivas Goud >> >> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning support for >> ZynqMP Platform >> >> On 21/02/18 17:00, Manish Narani wrote: >>> Hi Adrian, >>> >>>> -----Original Message----- >>>> From: Manish Narani >>>> Sent: Wednesday, February 21, 2018 11:39 AM >>>> To: Adrian Hunter ; michal.simek at xilinx.com; >>>> ulf.hansson at linaro.org; linux-arm-kernel at lists.infradead.org; linux- >>>> mmc at vger.kernel.org; linux-kernel at vger.kernel.org; >>>> devicetree at vger.kernel.org; mark.rutland at arm.com; >> robh+dt at kernel.org >>>> Cc: Anirudha Sarangi ; Srinivas Goud >>>> >>>> Subject: RE: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning >>>> support for ZynqMP Platform >>>> >>>> Hi Adrian, >>>> >>>> >>>>> -----Original Message----- >>>>> From: Adrian Hunter [mailto:adrian.hunter at intel.com] >>>>> Sent: Friday, February 16, 2018 7:37 PM >>>>> To: Manish Narani ; michal.simek at xilinx.com; >>>>> ulf.hansson at linaro.org; linux-arm-kernel at lists.infradead.org; linux- >>>>> mmc at vger.kernel.org; linux-kernel at vger.kernel.org; >>>>> devicetree at vger.kernel.org; mark.rutland at arm.com; >> robh+dt at kernel.org >>>>> Cc: Anirudha Sarangi ; Srinivas Goud >>>>> ; Manish Narani >>>>> Subject: Re: [RFC PATCH] mmc: sdhci-of-arasan: Add auto tuning >>>>> support for ZynqMP Platform >>>>> >>>>> On 30/01/18 20:14, Manish Narani wrote: >>>>>> This patch adds support of SD auto tuning for ZynqMP platform. Auto >>>>>> tuning sequence sends tuning block to card when operating in UHS-1 >>>>>> modes. This resets the DLL and sends CMD19/CMD21 as a part of the >>>>>> auto tuning process. Once the auto tuning process gets completed, >>>>>> reset the DLL to load the newly obtained SDHC tuned tap value. >>>>> >>>>> How is this different from: >>>>> 1. reset the dll >>>>> 2. call sdhci_execute_tuning >>>>> 3. reset the dll >>>>> >>> Below is my take on your above comments: >>> - 'Reset the dll' is a platform specific call inside >>> 'arasan_zynqmp_execute_tuning' which is implemented in >>> sdhci-of-arasan.c >>> - 'arasan_zynqmp_execute_tuning' is called from 'sdhci_execute_tuning' >>> as a platform_execute_tuning routine >>> - So to keep 'DLL reset' routine called from sdhci-of-arasan.c, I have >>> implemented the execute_tuning in sdhci-of-arasan.c >> >> I meant something like: >> >> if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp- >> 8.9a")) >> host->mmc_host_ops.execute_tuning = >> arasan_zynqmp_execute_tuning; >> > This will need the removal of 'const' from 'static const struct mmc_host_ops sdhci_ops = {}' in sdhci.c file. Please confirm. No, it is not const. You are not looking at the right place. i.e. $ grep mmc_host_ops drivers/mmc/host/sdhci.h struct mmc_host_ops mmc_host_ops; /* MMC host ops */ > > Thanks, > Manish >> >> static int arasan_zynqmp_execute_tuning(struct mmc_host *mmc, u32 >> opcode) { >> struct sdhci_host *host = mmc_priv(mmc); >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> struct sdhci_arasan_data *sdhci_arasan = >> sdhci_pltfm_priv(pltfm_host); >> int err; >> >> arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); >> >> err = sdhci_execute_tuning(mmc, opcode); >> if (err) >> return err; >> >> arasan_zynqmp_dll_reset(host, sdhci_arasan->device_id); >> >> return 0; >> } >> >>> >>> Alternative way (Please review): >>> - Define a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) in >>> sdhci-of-arasan.c indicating DLL reset needed while tuning operation >>> - Call 'dll reset' routine before and after __sdhci_execute_tuning() >>> in sdhci.c when a host->quirk2 bit (SDHCI_QUIRK2_DLL_RESET_NEEDED) is >>> set >> >> We should try to avoid quirks.