| Message ID | 1560247011-26369-4-git-send-email-manish.narani@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ZynqMP SD Clock Tap Delays configuration support | expand |
On 11/06/19 12:56 PM, Manish Narani wrote: > Apart from taps set by auto tuning, ZynqMP platform has feature to set > the tap values manually. Add support to read tap delay values from > DT and set the same in HW via ZynqMP SoC framework. Reading Tap > Delays from DT is optional, if the property is not available in DT the > driver will use the pre-defined Tap Delay Values. > > Signed-off-by: Manish Narani <manish.narani@xilinx.com> OK for SDHCI: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 172 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index b12abf9..7af6cec 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -22,6 +22,7 @@ > #include <linux/phy/phy.h> > #include <linux/regmap.h> > #include <linux/of.h> > +#include <linux/firmware/xlnx-zynqmp.h> > > #include "cqhci.h" > #include "sdhci-pltfm.h" > @@ -32,6 +33,10 @@ > > #define PHY_CLK_TOO_SLOW_HZ 400000 > > +/* Default settings for ZynqMP Tap Delays */ > +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} > +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} > + > /* > * On some SoCs the syscon area has a feature where the upper 16-bits of > * each 32-bit register act as a write mask for the lower 16-bits. This allows > @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { > * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. > * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > * @soc_ctl_map: Map to get offsets into soc_ctl registers. > + * @of_data: Platform specific runtime data storage pointer > */ > struct sdhci_arasan_data { > struct sdhci_host *host; > @@ -101,6 +107,15 @@ struct sdhci_arasan_data { > /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the > * internal clock even when the clock isn't stable */ > #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) > + > + void *of_data; > +}; > + > +struct sdhci_arasan_zynqmp_data { > + void (*set_tap_delay)(struct sdhci_host *host); > + const struct zynqmp_eemi_ops *eemi_ops; > + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ > + /* [1] for output delay */ > }; > > struct sdhci_arasan_of_data { > @@ -209,6 +224,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock) > sdhci_arasan->is_phy_on = false; > } > > + /* Set the Input and Output Tap Delays */ > + if (host->version >= SDHCI_SPEC_300 && > + host->timing != MMC_TIMING_LEGACY && > + host->timing != MMC_TIMING_UHS_SDR12) { > + struct sdhci_arasan_zynqmp_data *zynqmp_data = > + sdhci_arasan->of_data; > + if (zynqmp_data && zynqmp_data->set_tap_delay) > + zynqmp_data->set_tap_delay(host); > + } > + > sdhci_set_clock(host, clock); > > if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE) > @@ -487,6 +512,10 @@ static const struct of_device_id sdhci_arasan_of_match[] = { > .compatible = "arasan,sdhci-4.9a", > .data = &sdhci_arasan_data, > }, > + { > + .compatible = "xlnx,zynqmp-8.9a", > + .data = &sdhci_arasan_data, > + }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match); > @@ -517,6 +546,37 @@ static const struct clk_ops arasan_sdcardclk_ops = { > }; > > /** > + * sdhci_zynqmp_sdcardclk_set_phase - Set the SD Clock Tap Delays > + * > + * Set the SD Clock Tap Delays for Input and Output paths > + * > + * @hw: Pointer to the hardware clock structure. > + * @degrees The clock phase shift between 0 - 359. > + * Return: 0 on success and error value on error > + */ > +static int sdhci_zynqmp_sdcardclk_set_phase(struct clk_hw *hw, int degrees) > + > +{ > + struct sdhci_arasan_data *sdhci_arasan = > + container_of(hw, struct sdhci_arasan_data, sdcardclk_hw); > + struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data; > + const struct zynqmp_eemi_ops *eemi_ops = zynqmp_data->eemi_ops; > + const char *clk_name = clk_hw_get_name(hw); > + u32 device_id = !strcmp(clk_name, "clk_sd0") ? 0 : 1; > + > + if (!eemi_ops->sdio_setphase) > + return -ENODEV; > + > + /* Set the Clock Phase */ > + return eemi_ops->sdio_setphase(device_id, degrees); > +} > + > +static const struct clk_ops zynqmp_sdcardclk_ops = { > + .recalc_rate = sdhci_arasan_sdcardclk_recalc_rate, > + .set_phase = sdhci_zynqmp_sdcardclk_set_phase, > +}; > + > +/** > * sdhci_arasan_update_clockmultiplier - Set corecfg_clockmultiplier > * > * The corecfg_clockmultiplier is supposed to contain clock multiplier > @@ -638,7 +698,10 @@ static int sdhci_arasan_register_sdclk(struct sdhci_arasan_data *sdhci_arasan, > sdcardclk_init.parent_names = &parent_clk_name; > sdcardclk_init.num_parents = 1; > sdcardclk_init.flags = CLK_GET_RATE_NOCACHE; > - sdcardclk_init.ops = &arasan_sdcardclk_ops; > + if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a")) > + sdcardclk_init.ops = &zynqmp_sdcardclk_ops; > + else > + sdcardclk_init.ops = &arasan_sdcardclk_ops; > > sdhci_arasan->sdcardclk_hw.init = &sdcardclk_init; > sdhci_arasan->sdcardclk = > @@ -714,6 +777,108 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan) > return ret; > } > > +static void sdhci_arasan_zynqmp_set_tap_delay(struct sdhci_host *host) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > + struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data; > + > + clk_set_phase(sdhci_arasan->sdcardclk, > + (int)zynqmp_data->tapdly[host->timing][0]); > + clk_set_phase(sdhci_arasan->sdcardclk, > + (int)zynqmp_data->tapdly[host->timing][1] + > + INPUT_TAP_BOUNDARY); > +} > + > +static void arasan_dt_read_tap_delay(struct device *dev, u8 *tapdly, > + const char *prop, u8 itap_def, u8 otap_def) > +{ > + struct device_node *np = dev->of_node; > + > + tapdly[0] = itap_def; > + tapdly[1] = otap_def; > + > + /* > + * Read Tap Delay values from DT, if the DT does not contain the > + * Tap Values then use the pre-defined values. > + */ > + if (of_property_read_variable_u8_array(np, prop, &tapdly[0], 2, 0)) { > + dev_dbg(dev, "Using predefined tapdly for %s = %d %d\n", > + prop, tapdly[0], tapdly[1]); > + } > +} > + > +/** > + * arasan_dt_parse_tap_delays - Read Tap Delay values from DT > + * > + * Called at initialization to parse the values of Tap Delays. > + * > + * @dev: Pointer to our struct device. > + */ > +static int arasan_dt_parse_tap_delays(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct sdhci_host *host = platform_get_drvdata(pdev); > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > + struct sdhci_arasan_zynqmp_data zynqmp_data; > + const struct zynqmp_eemi_ops *eemi_ops; > + u8 *itapdly, *otapdly; > + u32 mio_bank = 0; > + > + eemi_ops = zynqmp_pm_get_eemi_ops(); > + if (IS_ERR(eemi_ops)) > + return PTR_ERR(eemi_ops); > + > + itapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS; > + otapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_OTAP_DELAYS; > + > + of_property_read_u32(pdev->dev.of_node, "xlnx,mio-bank", &mio_bank); > + if (mio_bank == 2) { > + otapdly[MMC_TIMING_UHS_SDR104] = 0x2; > + otapdly[MMC_TIMING_MMC_HS200] = 0x2; > + } > + > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS], > + "xlnx,tap-delay-mmc-hsd", > + itapdly[MMC_TIMING_MMC_HS], > + otapdly[MMC_TIMING_MMC_HS]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_SD_HS], > + "xlnx,tap-delay-sd-hsd", > + itapdly[MMC_TIMING_SD_HS], > + otapdly[MMC_TIMING_SD_HS]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR25], > + "xlnx,tap-delay-sdr25", > + itapdly[MMC_TIMING_UHS_SDR25], > + otapdly[MMC_TIMING_UHS_SDR25]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR50], > + "xlnx,tap-delay-sdr50", > + itapdly[MMC_TIMING_UHS_SDR50], > + otapdly[MMC_TIMING_UHS_SDR50]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR104], > + "xlnx,tap-delay-sdr104", > + itapdly[MMC_TIMING_UHS_SDR104], > + otapdly[MMC_TIMING_UHS_SDR104]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_DDR50], > + "xlnx,tap-delay-sd-ddr50", > + itapdly[MMC_TIMING_UHS_DDR50], > + otapdly[MMC_TIMING_UHS_DDR50]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_DDR52], > + "xlnx,tap-delay-mmc-ddr52", > + itapdly[MMC_TIMING_MMC_DDR52], > + otapdly[MMC_TIMING_MMC_DDR52]); > + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS200], > + "xlnx,tap-delay-mmc-hs200", > + itapdly[MMC_TIMING_MMC_HS200], > + otapdly[MMC_TIMING_MMC_HS200]); > + > + zynqmp_data.set_tap_delay = sdhci_arasan_zynqmp_set_tap_delay; > + zynqmp_data.eemi_ops = eemi_ops; > + sdhci_arasan->of_data = &zynqmp_data; > + > + return 0; > +} > + > static int sdhci_arasan_probe(struct platform_device *pdev) > { > int ret; > @@ -806,6 +971,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > goto unreg_clk; > } > > + if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-8.9a")) { > + ret = arasan_dt_parse_tap_delays(&pdev->dev); > + if (ret) > + goto unreg_clk; > + } > + > sdhci_arasan->phy = ERR_PTR(-ENODEV); > if (of_device_is_compatible(pdev->dev.of_node, > "arasan,sdhci-5.1")) { >
On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote: > > Apart from taps set by auto tuning, ZynqMP platform has feature to set > the tap values manually. Add support to read tap delay values from > DT and set the same in HW via ZynqMP SoC framework. Reading Tap > Delays from DT is optional, if the property is not available in DT the > driver will use the pre-defined Tap Delay Values. > > Signed-off-by: Manish Narani <manish.narani@xilinx.com> > --- > drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 172 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index b12abf9..7af6cec 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -22,6 +22,7 @@ > #include <linux/phy/phy.h> > #include <linux/regmap.h> > #include <linux/of.h> > +#include <linux/firmware/xlnx-zynqmp.h> > > #include "cqhci.h" > #include "sdhci-pltfm.h" > @@ -32,6 +33,10 @@ > > #define PHY_CLK_TOO_SLOW_HZ 400000 > > +/* Default settings for ZynqMP Tap Delays */ > +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} > +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} > + > /* > * On some SoCs the syscon area has a feature where the upper 16-bits of > * each 32-bit register act as a write mask for the lower 16-bits. This allows > @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { > * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. > * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > * @soc_ctl_map: Map to get offsets into soc_ctl registers. > + * @of_data: Platform specific runtime data storage pointer > */ > struct sdhci_arasan_data { > struct sdhci_host *host; > @@ -101,6 +107,15 @@ struct sdhci_arasan_data { > /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the > * internal clock even when the clock isn't stable */ > #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) > + > + void *of_data; > +}; > + > +struct sdhci_arasan_zynqmp_data { > + void (*set_tap_delay)(struct sdhci_host *host); > + const struct zynqmp_eemi_ops *eemi_ops; > + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ > + /* [1] for output delay */ > }; Please use two different structs, one for the clock provider data and one for the mmc variant/platform data. This makes the code more readable. 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. [...] Otherwise this looks good to me! When it comes to patch1, I need an ack from Michal to pick it up. Kind regards Uffe
Hi, On 17. 06. 19 13:15, Ulf Hansson wrote: > On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote: >> >> Apart from taps set by auto tuning, ZynqMP platform has feature to set >> the tap values manually. Add support to read tap delay values from >> DT and set the same in HW via ZynqMP SoC framework. Reading Tap >> Delays from DT is optional, if the property is not available in DT the >> driver will use the pre-defined Tap Delay Values. >> >> Signed-off-by: Manish Narani <manish.narani@xilinx.com> >> --- >> drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 172 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c >> index b12abf9..7af6cec 100644 >> --- a/drivers/mmc/host/sdhci-of-arasan.c >> +++ b/drivers/mmc/host/sdhci-of-arasan.c >> @@ -22,6 +22,7 @@ >> #include <linux/phy/phy.h> >> #include <linux/regmap.h> >> #include <linux/of.h> >> +#include <linux/firmware/xlnx-zynqmp.h> >> >> #include "cqhci.h" >> #include "sdhci-pltfm.h" >> @@ -32,6 +33,10 @@ >> >> #define PHY_CLK_TOO_SLOW_HZ 400000 >> >> +/* Default settings for ZynqMP Tap Delays */ >> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} >> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} >> + >> /* >> * On some SoCs the syscon area has a feature where the upper 16-bits of >> * each 32-bit register act as a write mask for the lower 16-bits. This allows >> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { >> * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. >> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. >> * @soc_ctl_map: Map to get offsets into soc_ctl registers. >> + * @of_data: Platform specific runtime data storage pointer >> */ >> struct sdhci_arasan_data { >> struct sdhci_host *host; >> @@ -101,6 +107,15 @@ struct sdhci_arasan_data { >> /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the >> * internal clock even when the clock isn't stable */ >> #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) >> + >> + void *of_data; >> +}; >> + >> +struct sdhci_arasan_zynqmp_data { >> + void (*set_tap_delay)(struct sdhci_host *host); >> + const struct zynqmp_eemi_ops *eemi_ops; >> + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ >> + /* [1] for output delay */ >> }; > > Please use two different structs, one for the clock provider data and > one for the mmc variant/platform data. This makes the code more > readable. Origin version before sending that out was using two fields. + u32 itapdly[MMC_TIMING_MMC_HS400 + 1]; + u32 otapdly[MMC_TIMING_MMC_HS400 + 1]; I did asked for putting it together to two dimensional array for improving readability of this code. The reason was that you need to take care about input/output together. One thing I was also suggesting was to use instead of 2 just enum values to specify IN=0/OUT/MAX to improve readability of this. Do you think that using enum should be enough? > 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 connection to another version of this driver for latest Xilinx chip it would be better to keep set_tap_delay callback in the driver. The reason is that new chip/ip is capable to setup tap delays directly without asking firmware to do it. That's why for versal IP there is a need to call different setup_tap_delay function. > > [...] > > Otherwise this looks good to me! > > When it comes to patch1, I need an ack from Michal to pick it up. I am waiting till Rob ack dt binding and then I wanted to talk to you if you want to take it with 1/3 or if you want me to take all of them via my tree. In previous releases I was taking them via my tree because there were several subsystem changing firmware interface. In this cycle there are just small changes to firmware interface that's why taking it via your tree shouldn't be a problem too. Thanks, Michal
On Mon, 17 Jun 2019 at 13:28, Michal Simek <michal.simek@xilinx.com> wrote: > > Hi, > > On 17. 06. 19 13:15, Ulf Hansson wrote: > > On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote: > >> > >> Apart from taps set by auto tuning, ZynqMP platform has feature to set > >> the tap values manually. Add support to read tap delay values from > >> DT and set the same in HW via ZynqMP SoC framework. Reading Tap > >> Delays from DT is optional, if the property is not available in DT the > >> driver will use the pre-defined Tap Delay Values. > >> > >> Signed-off-by: Manish Narani <manish.narani@xilinx.com> > >> --- > >> drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 172 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > >> index b12abf9..7af6cec 100644 > >> --- a/drivers/mmc/host/sdhci-of-arasan.c > >> +++ b/drivers/mmc/host/sdhci-of-arasan.c > >> @@ -22,6 +22,7 @@ > >> #include <linux/phy/phy.h> > >> #include <linux/regmap.h> > >> #include <linux/of.h> > >> +#include <linux/firmware/xlnx-zynqmp.h> > >> > >> #include "cqhci.h" > >> #include "sdhci-pltfm.h" > >> @@ -32,6 +33,10 @@ > >> > >> #define PHY_CLK_TOO_SLOW_HZ 400000 > >> > >> +/* Default settings for ZynqMP Tap Delays */ > >> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} > >> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} > >> + > >> /* > >> * On some SoCs the syscon area has a feature where the upper 16-bits of > >> * each 32-bit register act as a write mask for the lower 16-bits. This allows > >> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { > >> * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. > >> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > >> * @soc_ctl_map: Map to get offsets into soc_ctl registers. > >> + * @of_data: Platform specific runtime data storage pointer > >> */ > >> struct sdhci_arasan_data { > >> struct sdhci_host *host; > >> @@ -101,6 +107,15 @@ struct sdhci_arasan_data { > >> /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the > >> * internal clock even when the clock isn't stable */ > >> #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) > >> + > >> + void *of_data; > >> +}; > >> + > >> +struct sdhci_arasan_zynqmp_data { > >> + void (*set_tap_delay)(struct sdhci_host *host); > >> + const struct zynqmp_eemi_ops *eemi_ops; > >> + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ > >> + /* [1] for output delay */ > >> }; > > > > Please use two different structs, one for the clock provider data and > > one for the mmc variant/platform data. This makes the code more > > readable. > > Origin version before sending that out was using two fields. > + u32 itapdly[MMC_TIMING_MMC_HS400 + 1]; > + u32 otapdly[MMC_TIMING_MMC_HS400 + 1]; > > I did asked for putting it together to two dimensional array for > improving readability of this code. The reason was that you need to take > care about input/output together. > One thing I was also suggesting was to use instead of 2 just enum values > to specify IN=0/OUT/MAX to improve readability of this. > Do you think that using enum should be enough? Not sure I understand what you suggest here, sorry. I have no problem with the enums. The important point I am trying to make here, is that we should split the clock provider data and the mmc variant data, simply because those doesn't really belong to each each other. Something like this: struct sdhci_arasan_zynqmp_data { bool tap_delays; u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, [1] for output delay */ + other variant specific data one may want to put here } These are just regular mmc OF data that are parsed as any other property of the mmc device. The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into a clock provider specific struct, which is assigned when calling sdhci_arasan_register_sdclk. I understand that all the clock data is folded into struct sdhci_arasan_data today, but I think that should be moved into a "sub-struct" for the clock specifics. Moreover, when registering the clock, we should convert from using devm_clk_register() into devm_clk_hw_register() as the first one is now deprecated. > > > > 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 connection to another version of this driver for latest Xilinx chip > it would be better to keep set_tap_delay callback in the driver. The > reason is that new chip/ip is capable to setup tap delays directly > without asking firmware to do it. That's why for versal IP there is a > need to call different setup_tap_delay function. The ->set_tap_delay() callback is for ZyncMp pointing to sdhci_arasan_zynqmp_set_tap_delay(). This function calls the clk_set_phase() API. What does ->set_tap_delay() do for the latest version? > > > > > [...] > > > > Otherwise this looks good to me! > > > > When it comes to patch1, I need an ack from Michal to pick it up. > > I am waiting till Rob ack dt binding and then I wanted to talk to you if > you want to take it with 1/3 or if you want me to take all of them via > my tree. > In previous releases I was taking them via my tree because there were > several subsystem changing firmware interface. In this cycle there are > just small changes to firmware interface that's why taking it via your > tree shouldn't be a problem too. Okay, then let's target this via my mmc tree this time. Kind regards Uffe
On 17. 06. 19 14:21, Ulf Hansson wrote: > On Mon, 17 Jun 2019 at 13:28, Michal Simek <michal.simek@xilinx.com> wrote: >> >> Hi, >> >> On 17. 06. 19 13:15, Ulf Hansson wrote: >>> On Tue, 11 Jun 2019 at 11:57, Manish Narani <manish.narani@xilinx.com> wrote: >>>> >>>> Apart from taps set by auto tuning, ZynqMP platform has feature to set >>>> the tap values manually. Add support to read tap delay values from >>>> DT and set the same in HW via ZynqMP SoC framework. Reading Tap >>>> Delays from DT is optional, if the property is not available in DT the >>>> driver will use the pre-defined Tap Delay Values. >>>> >>>> Signed-off-by: Manish Narani <manish.narani@xilinx.com> >>>> --- >>>> drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 172 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c >>>> index b12abf9..7af6cec 100644 >>>> --- a/drivers/mmc/host/sdhci-of-arasan.c >>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c >>>> @@ -22,6 +22,7 @@ >>>> #include <linux/phy/phy.h> >>>> #include <linux/regmap.h> >>>> #include <linux/of.h> >>>> +#include <linux/firmware/xlnx-zynqmp.h> >>>> >>>> #include "cqhci.h" >>>> #include "sdhci-pltfm.h" >>>> @@ -32,6 +33,10 @@ >>>> >>>> #define PHY_CLK_TOO_SLOW_HZ 400000 >>>> >>>> +/* Default settings for ZynqMP Tap Delays */ >>>> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} >>>> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} >>>> + >>>> /* >>>> * On some SoCs the syscon area has a feature where the upper 16-bits of >>>> * each 32-bit register act as a write mask for the lower 16-bits. This allows >>>> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { >>>> * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. >>>> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. >>>> * @soc_ctl_map: Map to get offsets into soc_ctl registers. >>>> + * @of_data: Platform specific runtime data storage pointer >>>> */ >>>> struct sdhci_arasan_data { >>>> struct sdhci_host *host; >>>> @@ -101,6 +107,15 @@ struct sdhci_arasan_data { >>>> /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the >>>> * internal clock even when the clock isn't stable */ >>>> #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) >>>> + >>>> + void *of_data; >>>> +}; >>>> + >>>> +struct sdhci_arasan_zynqmp_data { >>>> + void (*set_tap_delay)(struct sdhci_host *host); >>>> + const struct zynqmp_eemi_ops *eemi_ops; >>>> + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ >>>> + /* [1] for output delay */ >>>> }; >>> >>> Please use two different structs, one for the clock provider data and >>> one for the mmc variant/platform data. This makes the code more >>> readable. >> >> Origin version before sending that out was using two fields. >> + u32 itapdly[MMC_TIMING_MMC_HS400 + 1]; >> + u32 otapdly[MMC_TIMING_MMC_HS400 + 1]; >> >> I did asked for putting it together to two dimensional array for >> improving readability of this code. The reason was that you need to take >> care about input/output together. >> One thing I was also suggesting was to use instead of 2 just enum values >> to specify IN=0/OUT/MAX to improve readability of this. >> Do you think that using enum should be enough? > > Not sure I understand what you suggest here, sorry. I have no problem > with the enums. > > The important point I am trying to make here, is that we should split > the clock provider data and the mmc variant data, simply because those > doesn't really belong to each each other. > > Something like this: > > struct sdhci_arasan_zynqmp_data { > bool tap_delays; > u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input > delay, [1] for output delay */ > + other variant specific data one may want to put here > } > > These are just regular mmc OF data that are parsed as any other > property of the mmc device. > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into > a clock provider specific struct, which is assigned when calling > sdhci_arasan_register_sdclk. I understand that all the clock data is > folded into struct sdhci_arasan_data today, but I think that should be > moved into a "sub-struct" for the clock specifics. > > Moreover, when registering the clock, we should convert from using > devm_clk_register() into devm_clk_hw_register() as the first one is > now deprecated. Ok. I got your point. >> >> >>> 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 connection to another version of this driver for latest Xilinx chip >> it would be better to keep set_tap_delay callback in the driver. The >> reason is that new chip/ip is capable to setup tap delays directly >> without asking firmware to do it. That's why for versal IP there is a >> need to call different setup_tap_delay function. > > The ->set_tap_delay() callback is for ZyncMp pointing to > sdhci_arasan_zynqmp_set_tap_delay(). This function calls the > clk_set_phase() API. > > What does ->set_tap_delay() do for the latest version? There is different set of default tap delays which should be programmed and it is done just via writing to registers which are the part of controller address space. >> >>> >>> [...] >>> >>> Otherwise this looks good to me! >>> >>> When it comes to patch1, I need an ack from Michal to pick it up. >> >> I am waiting till Rob ack dt binding and then I wanted to talk to you if >> you want to take it with 1/3 or if you want me to take all of them via >> my tree. >> In previous releases I was taking them via my tree because there were >> several subsystem changing firmware interface. In this cycle there are >> just small changes to firmware interface that's why taking it via your >> tree shouldn't be a problem too. > > Okay, then let's target this via my mmc tree this time. okay. Not a problem. Thanks, Michal
[...] > >> > >> > >>> 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 connection to another version of this driver for latest Xilinx chip > >> it would be better to keep set_tap_delay callback in the driver. The > >> reason is that new chip/ip is capable to setup tap delays directly > >> without asking firmware to do it. That's why for versal IP there is a > >> need to call different setup_tap_delay function. > > > > The ->set_tap_delay() callback is for ZyncMp pointing to > > sdhci_arasan_zynqmp_set_tap_delay(). This function calls the > > clk_set_phase() API. > > > > What does ->set_tap_delay() do for the latest version? > > There is different set of default tap delays which should be programmed > and it is done just via writing to registers which are the part of > controller address space. Okay, I see. Not sure what makes most sense to do here, but it sounds to me like another ->set_phase() callback should be implemented for the clock provider. In other words, calling clk_set_phase() should continue to works just fine for this case as well. If it turns out to be inconvenient, we can always add the ->set_tap_delay() at a later point when it makes more sense. [...] Kind regards Uffe
Hi Uffe, Thanks for the review. Please find my comments below. > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Monday, June 17, 2019 8:29 PM > To: Michal Simek <michals@xilinx.com> > Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Adrian > Hunter <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly > Shah <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@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? Thanks, Manish > > >> > > >> In connection to another version of this driver for latest Xilinx chip > > >> it would be better to keep set_tap_delay callback in the driver. The > > >> reason is that new chip/ip is capable to setup tap delays directly > > >> without asking firmware to do it. That's why for versal IP there is a > > >> need to call different setup_tap_delay function. > > > > > > The ->set_tap_delay() callback is for ZyncMp pointing to > > > sdhci_arasan_zynqmp_set_tap_delay(). This function calls the > > > clk_set_phase() API. > > > > > > What does ->set_tap_delay() do for the latest version? > > > > There is different set of default tap delays which should be programmed > > and it is done just via writing to registers which are the part of > > controller address space. > > Okay, I see. > > Not sure what makes most sense to do here, but it sounds to me like > another ->set_phase() callback should be implemented for the clock > provider. In other words, calling clk_set_phase() should continue to > works just fine for this case as well. If it turns out to be > inconvenient, we can always add the ->set_tap_delay() at a later point > when it makes more sense. > > [...] > > Kind regards > Uffe
Hi Uffe, > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Monday, June 17, 2019 5:51 PM [...] > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into > a clock provider specific struct, which is assigned when calling > sdhci_arasan_register_sdclk. I understand that all the clock data is > folded into struct sdhci_arasan_data today, but I think that should be > moved into a "sub-struct" for the clock specifics. > > Moreover, when registering the clock, we should convert from using > devm_clk_register() into devm_clk_hw_register() as the first one is > now deprecated. Just a query here: When we switch to using devm_clk_hw_register() here, it will register the clk_hw and return int. Is there a way we can get the clk (related to the clk_hw registered) from the clock framework? I am asking this because we will need that clk pointer while calling clk_set_phase() function. Thanks, Manish
On Wed, 19 Jun 2019 at 10:40, Manish Narani <MNARANI@xilinx.com> wrote: > > Hi Uffe, > > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Monday, June 17, 2019 5:51 PM > [...] > > > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into > > a clock provider specific struct, which is assigned when calling > > sdhci_arasan_register_sdclk. I understand that all the clock data is > > folded into struct sdhci_arasan_data today, but I think that should be > > moved into a "sub-struct" for the clock specifics. > > > > Moreover, when registering the clock, we should convert from using > > devm_clk_register() into devm_clk_hw_register() as the first one is > > now deprecated. > > Just a query here: > When we switch to using devm_clk_hw_register() here, it will register the clk_hw and return int. > Is there a way we can get the clk (related to the clk_hw registered) from the > clock framework? > I am asking this because we will need that clk pointer while calling clk_set_phase() function. I assume devm_clk_get() should work fine? Kind regards Uffe
On Tue, 18 Jun 2019 at 06:59, Manish Narani <MNARANI@xilinx.com> wrote: > > Hi Uffe, > > Thanks for the review. Please find my comments below. > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Monday, June 17, 2019 8:29 PM > > To: Michal Simek <michals@xilinx.com> > > Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring > > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Adrian > > Hunter <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly > > Shah <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof > > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML > > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@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? [...] Kind regards Uffe
Hi Uffe, > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Wednesday, June 19, 2019 7:09 PM > To: Manish Narani <MNARANI@xilinx.com> > Cc: Michal Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>; > Mark Rutland <mark.rutland@arm.com>; Adrian Hunter > <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah > <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP > Platform Tap Delays Setup > > On Wed, 19 Jun 2019 at 10:40, Manish Narani <MNARANI@xilinx.com> wrote: > > > > Hi Uffe, > > > > > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > Sent: Monday, June 17, 2019 5:51 PM > > [...] > > > > > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into > > > a clock provider specific struct, which is assigned when calling > > > sdhci_arasan_register_sdclk. I understand that all the clock data is > > > folded into struct sdhci_arasan_data today, but I think that should be > > > moved into a "sub-struct" for the clock specifics. > > > > > > Moreover, when registering the clock, we should convert from using > > > devm_clk_register() into devm_clk_hw_register() as the first one is > > > now deprecated. > > > > Just a query here: > > When we switch to using devm_clk_hw_register() here, it will register the > clk_hw and return int. > > Is there a way we can get the clk (related to the clk_hw registered) from the > > clock framework? > > I am asking this because we will need that clk pointer while calling > clk_set_phase() function. > > I assume devm_clk_get() should work fine? This clock does not come through ZynqMP Clock framework. We are initializing it in this 'sdhci-of-arasan' driver and getting only the clock name from "clock_output_names" property. So I think devm_clk_get() will not work here for our case. I have gone through the clock framework and I found one function which may be used to create clock from clock hw, that is ' clk_hw_create_clk()' which can be used from our driver, however this needs change in the clock framework as below : --- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index aa51756..4dc69ff 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3420,6 +3420,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, return clk; } +EXPORT_SYMBOL_GPL(clk_hw_create_clk); static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) { diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h index d8400d6..2319899 100644 --- a/drivers/clk/clk.h +++ b/drivers/clk/clk.h @@ -22,17 +22,9 @@ static inline struct clk_hw *of_clk_get_hw(struct device_node *np, struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id); #ifdef CONFIG_COMMON_CLK -struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, - const char *dev_id, const char *con_id); void __clk_put(struct clk *clk); #else /* All these casts to avoid ifdefs in clkdev... */ -static inline struct clk * -clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id, - const char *con_id) -{ - return (struct clk *)hw; -} static struct clk_hw *__clk_get_hw(struct clk *clk) { return (struct clk_hw *)clk; diff --git a/include/linux/clk.h b/include/linux/clk.h index f689fc5..d3f60fe 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -18,6 +18,7 @@ struct device; struct clk; +struct clk_hw; struct device_node; struct of_phandle_args; @@ -934,4 +935,15 @@ static inline struct clk *of_clk_get_from_provider(struct of_phandle_args *clksp } #endif +#ifdef CONFIG_COMMON_CLK +struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, + const char *dev_id, const char *con_id); +#else +static inline struct clk * +clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id, + const char *con_id) +{ + return (struct clk *)hw; +} +#endif #endif --- This change should help other drivers (outside 'drivers/clk/') as well for getting the clock created from clk_hw. Is this fine to do? Thanks, Manish
Hi Uffe, > -----Original Message----- > From: Ulf Hansson <ulf.hansson@linaro.org> > Sent: Wednesday, June 19, 2019 8:11 PM > To: Manish Narani <MNARANI@xilinx.com> > Cc: Michal Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>; > Mark Rutland <mark.rutland@arm.com>; Adrian Hunter > <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah > <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@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 <MNARANI@xilinx.com> wrote: > > > > Hi Uffe, > > > > Thanks for the review. Please find my comments below. > > > > > -----Original Message----- > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > Sent: Monday, June 17, 2019 8:29 PM > > > To: Michal Simek <michals@xilinx.com> > > > Cc: Manish Narani <MNARANI@xilinx.com>; Rob Herring > > > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Adrian > > > Hunter <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly > > > Shah <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; > Olof > > > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML > > > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > > > kernel@vger.kernel.org>; Linux ARM <linux-arm- > kernel@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
On Thu, 20 Jun 2019 at 10:14, Manish Narani <MNARANI@xilinx.com> wrote: > > Hi Uffe, > > > > -----Original Message----- > > From: Ulf Hansson <ulf.hansson@linaro.org> > > Sent: Wednesday, June 19, 2019 7:09 PM > > To: Manish Narani <MNARANI@xilinx.com> > > Cc: Michal Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>; > > Mark Rutland <mark.rutland@arm.com>; Adrian Hunter > > <adrian.hunter@intel.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah > > <JOLLYS@xilinx.com>; Nava kishore Manne <navam@xilinx.com>; Olof > > Johansson <olof@lixom.net>; linux-mmc@vger.kernel.org; DTML > > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > > kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org> > > Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP > > Platform Tap Delays Setup > > > > On Wed, 19 Jun 2019 at 10:40, Manish Narani <MNARANI@xilinx.com> wrote: > > > > > > Hi Uffe, > > > > > > > > > > -----Original Message----- > > > > From: Ulf Hansson <ulf.hansson@linaro.org> > > > > Sent: Monday, June 17, 2019 5:51 PM > > > [...] > > > > > > > > The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into > > > > a clock provider specific struct, which is assigned when calling > > > > sdhci_arasan_register_sdclk. I understand that all the clock data is > > > > folded into struct sdhci_arasan_data today, but I think that should be > > > > moved into a "sub-struct" for the clock specifics. > > > > > > > > Moreover, when registering the clock, we should convert from using > > > > devm_clk_register() into devm_clk_hw_register() as the first one is > > > > now deprecated. > > > > > > Just a query here: > > > When we switch to using devm_clk_hw_register() here, it will register the > > clk_hw and return int. > > > Is there a way we can get the clk (related to the clk_hw registered) from the > > > clock framework? > > > I am asking this because we will need that clk pointer while calling > > clk_set_phase() function. > > > > I assume devm_clk_get() should work fine? > > This clock does not come through ZynqMP Clock framework. We are initializing it in this 'sdhci-of-arasan' driver and getting only the clock name from "clock_output_names" property. So I think devm_clk_get() will not work here for our case. Well, I guess you need to register an OF clock provider to allow the clock lookup to work. Apologize, but I don't have the time, currently to point you in the exact direction. However, in principle, my point is, there should be no difference whether the clock is registered via the "ZynqMP Clock framework" or via the mmc driver. The *clk_get() thing need to work, otherwise I consider the clock registration in the mmc driver to be a hack. If you see what I mean. > I have gone through the clock framework and I found one function which may be used to create clock from clock hw, that is ' clk_hw_create_clk()' which can be used from our driver, however this needs change in the clock framework as below : > > --- > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index aa51756..4dc69ff 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3420,6 +3420,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > > return clk; > } > +EXPORT_SYMBOL_GPL(clk_hw_create_clk); > > static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist) > { > diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h > index d8400d6..2319899 100644 > --- a/drivers/clk/clk.h > +++ b/drivers/clk/clk.h > @@ -22,17 +22,9 @@ static inline struct clk_hw *of_clk_get_hw(struct device_node *np, > struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id); > > #ifdef CONFIG_COMMON_CLK > -struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > - const char *dev_id, const char *con_id); > void __clk_put(struct clk *clk); > #else > /* All these casts to avoid ifdefs in clkdev... */ > -static inline struct clk * > -clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id, > - const char *con_id) > -{ > - return (struct clk *)hw; > -} > static struct clk_hw *__clk_get_hw(struct clk *clk) > { > return (struct clk_hw *)clk; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index f689fc5..d3f60fe 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -18,6 +18,7 @@ > > struct device; > struct clk; > +struct clk_hw; > struct device_node; > struct of_phandle_args; > > @@ -934,4 +935,15 @@ static inline struct clk *of_clk_get_from_provider(struct of_phandle_args *clksp > } > #endif > > +#ifdef CONFIG_COMMON_CLK > +struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > + const char *dev_id, const char *con_id); > +#else > +static inline struct clk * > +clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id, > + const char *con_id) > +{ > + return (struct clk *)hw; > +} > +#endif > #endif > --- > > This change should help other drivers (outside 'drivers/clk/') as well for getting the clock created from clk_hw. > Is this fine to do? I think this is the wrong approach, see why further above. Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index b12abf9..7af6cec 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -22,6 +22,7 @@ #include <linux/phy/phy.h> #include <linux/regmap.h> #include <linux/of.h> +#include <linux/firmware/xlnx-zynqmp.h> #include "cqhci.h" #include "sdhci-pltfm.h" @@ -32,6 +33,10 @@ #define PHY_CLK_TOO_SLOW_HZ 400000 +/* Default settings for ZynqMP Tap Delays */ +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} + /* * On some SoCs the syscon area has a feature where the upper 16-bits of * each 32-bit register act as a write mask for the lower 16-bits. This allows @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. * @soc_ctl_map: Map to get offsets into soc_ctl registers. + * @of_data: Platform specific runtime data storage pointer */ struct sdhci_arasan_data { struct sdhci_host *host; @@ -101,6 +107,15 @@ struct sdhci_arasan_data { /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the * internal clock even when the clock isn't stable */ #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) + + void *of_data; +}; + +struct sdhci_arasan_zynqmp_data { + void (*set_tap_delay)(struct sdhci_host *host); + const struct zynqmp_eemi_ops *eemi_ops; + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ + /* [1] for output delay */ }; struct sdhci_arasan_of_data { @@ -209,6 +224,16 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock) sdhci_arasan->is_phy_on = false; } + /* Set the Input and Output Tap Delays */ + if (host->version >= SDHCI_SPEC_300 && + host->timing != MMC_TIMING_LEGACY && + host->timing != MMC_TIMING_UHS_SDR12) { + struct sdhci_arasan_zynqmp_data *zynqmp_data = + sdhci_arasan->of_data; + if (zynqmp_data && zynqmp_data->set_tap_delay) + zynqmp_data->set_tap_delay(host); + } + sdhci_set_clock(host, clock); if (sdhci_arasan->quirks & SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE) @@ -487,6 +512,10 @@ static const struct of_device_id sdhci_arasan_of_match[] = { .compatible = "arasan,sdhci-4.9a", .data = &sdhci_arasan_data, }, + { + .compatible = "xlnx,zynqmp-8.9a", + .data = &sdhci_arasan_data, + }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match); @@ -517,6 +546,37 @@ static const struct clk_ops arasan_sdcardclk_ops = { }; /** + * sdhci_zynqmp_sdcardclk_set_phase - Set the SD Clock Tap Delays + * + * Set the SD Clock Tap Delays for Input and Output paths + * + * @hw: Pointer to the hardware clock structure. + * @degrees The clock phase shift between 0 - 359. + * Return: 0 on success and error value on error + */ +static int sdhci_zynqmp_sdcardclk_set_phase(struct clk_hw *hw, int degrees) + +{ + struct sdhci_arasan_data *sdhci_arasan = + container_of(hw, struct sdhci_arasan_data, sdcardclk_hw); + struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data; + const struct zynqmp_eemi_ops *eemi_ops = zynqmp_data->eemi_ops; + const char *clk_name = clk_hw_get_name(hw); + u32 device_id = !strcmp(clk_name, "clk_sd0") ? 0 : 1; + + if (!eemi_ops->sdio_setphase) + return -ENODEV; + + /* Set the Clock Phase */ + return eemi_ops->sdio_setphase(device_id, degrees); +} + +static const struct clk_ops zynqmp_sdcardclk_ops = { + .recalc_rate = sdhci_arasan_sdcardclk_recalc_rate, + .set_phase = sdhci_zynqmp_sdcardclk_set_phase, +}; + +/** * sdhci_arasan_update_clockmultiplier - Set corecfg_clockmultiplier * * The corecfg_clockmultiplier is supposed to contain clock multiplier @@ -638,7 +698,10 @@ static int sdhci_arasan_register_sdclk(struct sdhci_arasan_data *sdhci_arasan, sdcardclk_init.parent_names = &parent_clk_name; sdcardclk_init.num_parents = 1; sdcardclk_init.flags = CLK_GET_RATE_NOCACHE; - sdcardclk_init.ops = &arasan_sdcardclk_ops; + if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a")) + sdcardclk_init.ops = &zynqmp_sdcardclk_ops; + else + sdcardclk_init.ops = &arasan_sdcardclk_ops; sdhci_arasan->sdcardclk_hw.init = &sdcardclk_init; sdhci_arasan->sdcardclk = @@ -714,6 +777,108 @@ static int sdhci_arasan_add_host(struct sdhci_arasan_data *sdhci_arasan) return ret; } +static void sdhci_arasan_zynqmp_set_tap_delay(struct sdhci_host *host) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + struct sdhci_arasan_zynqmp_data *zynqmp_data = sdhci_arasan->of_data; + + clk_set_phase(sdhci_arasan->sdcardclk, + (int)zynqmp_data->tapdly[host->timing][0]); + clk_set_phase(sdhci_arasan->sdcardclk, + (int)zynqmp_data->tapdly[host->timing][1] + + INPUT_TAP_BOUNDARY); +} + +static void arasan_dt_read_tap_delay(struct device *dev, u8 *tapdly, + const char *prop, u8 itap_def, u8 otap_def) +{ + struct device_node *np = dev->of_node; + + tapdly[0] = itap_def; + tapdly[1] = otap_def; + + /* + * Read Tap Delay values from DT, if the DT does not contain the + * Tap Values then use the pre-defined values. + */ + if (of_property_read_variable_u8_array(np, prop, &tapdly[0], 2, 0)) { + dev_dbg(dev, "Using predefined tapdly for %s = %d %d\n", + prop, tapdly[0], tapdly[1]); + } +} + +/** + * arasan_dt_parse_tap_delays - Read Tap Delay values from DT + * + * Called at initialization to parse the values of Tap Delays. + * + * @dev: Pointer to our struct device. + */ +static int arasan_dt_parse_tap_delays(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct sdhci_host *host = platform_get_drvdata(pdev); + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + struct sdhci_arasan_zynqmp_data zynqmp_data; + const struct zynqmp_eemi_ops *eemi_ops; + u8 *itapdly, *otapdly; + u32 mio_bank = 0; + + eemi_ops = zynqmp_pm_get_eemi_ops(); + if (IS_ERR(eemi_ops)) + return PTR_ERR(eemi_ops); + + itapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_ITAP_DELAYS; + otapdly = (u8 [MMC_TIMING_MMC_HS400 + 1]) ZYNQMP_OTAP_DELAYS; + + of_property_read_u32(pdev->dev.of_node, "xlnx,mio-bank", &mio_bank); + if (mio_bank == 2) { + otapdly[MMC_TIMING_UHS_SDR104] = 0x2; + otapdly[MMC_TIMING_MMC_HS200] = 0x2; + } + + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS], + "xlnx,tap-delay-mmc-hsd", + itapdly[MMC_TIMING_MMC_HS], + otapdly[MMC_TIMING_MMC_HS]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_SD_HS], + "xlnx,tap-delay-sd-hsd", + itapdly[MMC_TIMING_SD_HS], + otapdly[MMC_TIMING_SD_HS]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR25], + "xlnx,tap-delay-sdr25", + itapdly[MMC_TIMING_UHS_SDR25], + otapdly[MMC_TIMING_UHS_SDR25]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR50], + "xlnx,tap-delay-sdr50", + itapdly[MMC_TIMING_UHS_SDR50], + otapdly[MMC_TIMING_UHS_SDR50]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_SDR104], + "xlnx,tap-delay-sdr104", + itapdly[MMC_TIMING_UHS_SDR104], + otapdly[MMC_TIMING_UHS_SDR104]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_UHS_DDR50], + "xlnx,tap-delay-sd-ddr50", + itapdly[MMC_TIMING_UHS_DDR50], + otapdly[MMC_TIMING_UHS_DDR50]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_DDR52], + "xlnx,tap-delay-mmc-ddr52", + itapdly[MMC_TIMING_MMC_DDR52], + otapdly[MMC_TIMING_MMC_DDR52]); + arasan_dt_read_tap_delay(dev, zynqmp_data.tapdly[MMC_TIMING_MMC_HS200], + "xlnx,tap-delay-mmc-hs200", + itapdly[MMC_TIMING_MMC_HS200], + otapdly[MMC_TIMING_MMC_HS200]); + + zynqmp_data.set_tap_delay = sdhci_arasan_zynqmp_set_tap_delay; + zynqmp_data.eemi_ops = eemi_ops; + sdhci_arasan->of_data = &zynqmp_data; + + return 0; +} + static int sdhci_arasan_probe(struct platform_device *pdev) { int ret; @@ -806,6 +971,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev) goto unreg_clk; } + if (of_device_is_compatible(pdev->dev.of_node, "xlnx,zynqmp-8.9a")) { + ret = arasan_dt_parse_tap_delays(&pdev->dev); + if (ret) + goto unreg_clk; + } + sdhci_arasan->phy = ERR_PTR(-ENODEV); if (of_device_is_compatible(pdev->dev.of_node, "arasan,sdhci-5.1")) {
Apart from taps set by auto tuning, ZynqMP platform has feature to set the tap values manually. Add support to read tap delay values from DT and set the same in HW via ZynqMP SoC framework. Reading Tap Delays from DT is optional, if the property is not available in DT the driver will use the pre-defined Tap Delay Values. Signed-off-by: Manish Narani <manish.narani@xilinx.com> --- drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- 1 file changed, 172 insertions(+), 1 deletion(-)