Hi Uffe, > -----Original Message----- > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > Sent: Wednesday, June 19, 2019 8:11 PM > To: Manish Narani <MNARANI@xxxxxxxxxx> > Cc: Michal Simek <michals@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; > Mark Rutland <mark.rutland@xxxxxxx>; Adrian Hunter > <adrian.hunter@xxxxxxxxx>; Rajan Vaja <RAJANV@xxxxxxxxxx>; Jolly Shah > <JOLLYS@xxxxxxxxxx>; Nava kishore Manne <navam@xxxxxxxxxx>; Olof > Johansson <olof@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; DTML > <devicetree@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux- > kernel@xxxxxxxxxxxxxxx>; Linux ARM <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx> > 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 <MNARANI@xxxxxxxxxx> wrote: > > > > Hi Uffe, > > > > Thanks for the review. Please find my comments below. > > > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > > > Sent: Monday, June 17, 2019 8:29 PM > > > To: Michal Simek <michals@xxxxxxxxxx> > > > Cc: Manish Narani <MNARANI@xxxxxxxxxx>; Rob Herring > > > <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Adrian > > > Hunter <adrian.hunter@xxxxxxxxx>; Rajan Vaja <RAJANV@xxxxxxxxxx>; Jolly > > > Shah <JOLLYS@xxxxxxxxxx>; Nava kishore Manne <navam@xxxxxxxxxx>; > Olof > > > Johansson <olof@xxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; DTML > > > <devicetree@xxxxxxxxxxxxxxx>; Linux Kernel Mailing List <linux- > > > kernel@xxxxxxxxxxxxxxx>; Linux ARM <linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx> > > > 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
