lkml.org 
[lkml]   [2020]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 1/1] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
From
Date
Hi, +Arnd,

On 14. 09. 20 7:12, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>
> Voltage switching sequence is needed to support UHS-1 interface
> as Keem Bay EVM is using external voltage regulator to switch between
> 1.8V and 3.3V.
>
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-of-arasan.c | 140 +++++++++++++++++++++++++++++
> 1 file changed, 140 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index f186fbd016b1..c133408d0c74 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -16,7 +16,9 @@
> */
>
> #include <linux/clk-provider.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/mfd/syscon.h>
> +#include <linux/arm-smccc.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/phy/phy.h>
> @@ -41,6 +43,11 @@
> #define SDHCI_ITAPDLY_ENABLE 0x100
> #define SDHCI_OTAPDLY_ENABLE 0x40
>
> +/* Setting for Keem Bay IO Pad 1.8 Voltage Selection */
> +#define KEEMBAY_AON_SIP_FUNC_ID 0x8200ff26
> +#define KEEMBAY_AON_SET_1V8_VOLT 0x01
> +#define KEEMBAY_AON_SET_3V3_VOLT 0x00
> +
> /* Default settings for ZynqMP Clock Phases */
> #define ZYNQMP_ICLK_PHASE {0, 63, 63, 0, 63, 0, 0, 183, 54, 0, 0}
> #define ZYNQMP_OCLK_PHASE {0, 72, 60, 0, 60, 72, 135, 48, 72, 135, 0}
> @@ -150,6 +157,7 @@ struct sdhci_arasan_data {
> struct regmap *soc_ctl_base;
> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
> unsigned int quirks;
> + struct gpio_desc *uhs_gpio;
>
> /* Controller does not have CD wired and will not function normally without */
> #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST BIT(0)
> @@ -361,6 +369,121 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
> return -EINVAL;
> }
>
> +static int sdhci_arasan_keembay_set_voltage(int volt)
> +{
> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC)
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(KEEMBAY_AON_SIP_FUNC_ID, volt, 0, 0, 0, 0, 0, 0, &res);
> + if (res.a0)
> + return -EINVAL;
> + return 0;

I am just curious about calling this smc directly from device driver. I
see that several drivers are doing this but isn't it better to hide
these in firmware driver?
Also the part of FUNC_ID is smc32, sip service call (0x82000000)
function identifier which is likely something what should be used as
macro in shared location that others can use it too.

Another part is that based on description you are talking to external
voltage regulator without using regulator framework at all. Isn't it
better just to create firmware based regulator for this purpose?

Or was there any agreement to put these stuff directly to the driver?

Thanks,
Michal

\
 
 \ /
  Last update: 2020-09-14 08:46    [W:0.095 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site