Hi Uffe,
> -----Original Message----- > From: Ulf Hansson <ulf.hans...@linaro.org> > Sent: Wednesday, June 19, 2019 8:11 PM > To: Manish Narani <mnar...@xilinx.com> > Cc: Michal Simek <mich...@xilinx.com>; Rob Herring <robh...@kernel.org>; > Mark Rutland <mark.rutl...@arm.com>; Adrian Hunter > <adrian.hun...@intel.com>; Rajan Vaja <raj...@xilinx.com>; Jolly Shah > <jol...@xilinx.com>; Nava kishore Manne <na...@xilinx.com>; Olof > Johansson <o...@lixom.net>; linux-...@vger.kernel.org; DTML > <devicet...@vger.kernel.org>; Linux Kernel Mailing List <linux- > ker...@vger.kernel.org>; Linux ARM <linux-arm-ker...@lists.infradead.org> > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP > Platform Tap Delays Setup > > On Tue, 18 Jun 2019 at 06:59, Manish Narani <mnar...@xilinx.com> wrote: > > > > Hi Uffe, > > > > Thanks for the review. Please find my comments below. > > > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hans...@linaro.org> > > > Sent: Monday, June 17, 2019 8:29 PM > > > To: Michal Simek <mich...@xilinx.com> > > > Cc: Manish Narani <mnar...@xilinx.com>; Rob Herring > > > <robh...@kernel.org>; Mark Rutland <mark.rutl...@arm.com>; Adrian > > > Hunter <adrian.hun...@intel.com>; Rajan Vaja <raj...@xilinx.com>; Jolly > > > Shah <jol...@xilinx.com>; Nava kishore Manne <na...@xilinx.com>; > Olof > > > Johansson <o...@lixom.net>; linux-...@vger.kernel.org; DTML > > > <devicet...@vger.kernel.org>; Linux Kernel Mailing List <linux- > > > ker...@vger.kernel.org>; Linux ARM <linux-arm- > ker...@lists.infradead.org> > > > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP > > > Platform Tap Delays Setup > > > > > > [...] > > > > > > > >> > > > > >> > > > > >>> In regards to the mmc data part, I suggest to drop the > > > > >>> ->set_tap_delay() callback, but rather use a boolean flag to > > > > >>> indicate > > > > >>> whether clock phases needs to be changed for the variant. > > > > >>> Potentially > > > > >>> that could even be skipped and instead call clk_set_phase() > > > > >>> unconditionally, as the clock core deals fine with clock providers > > > > >>> that doesn't support the ->set_phase() callback. > > > > In the current implementation, I am taking care of both the input and > > output clock delays with the single clock (which is output clock) > > registration > > and differentiating these tap delays based on their values > > (<256 then input delay and >= 256 then output delay), because that is > > zynqmp specific. If we want to make this generic, we may need to > > register 'another' clock which will be there as an input (sampling) clock > > and then we can make this 'clk_set_phase()' be called unconditionally > > each for both the clocks and let the platforms handle their clock part. > > What's your take on this? > > Not sure exactly what you are suggesting, but my gut feeling says it > sounds good. > > How is tap delays managed for both the input clock and the output > clock? Is some managed by the clock provider (which is probably > firmware in your case) and some managed by the MMC controller? Yes, for the existing "xlnx,zynqmp-8.9a" controller, the tap delays will be managed by the firmware, however in the upcoming "xlnx,versal-8.9a" variant the tap delays will be managed by the MMC controller itself. I will include the Versal one in the next series of patches. Thanks, Manish
