mbox series

[v4,0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

Message ID 20201008020936.19894-1-muhammad.husaini.zulkifli@intel.com
Headers show
Series mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC | expand

Message

Zulkifli, Muhammad Husaini Oct. 8, 2020, 2:09 a.m. UTC
From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

Hi.

The first patch is the header file to handle ATF call.

The second patch is DT binding for arasan controller for uhs support.

The third patch is to introduce the structure device pointer in arasan controller probe.

The fourth patch is to enable UHS-1 Support for Keem Bay EVM.

All of these patches was tested with Keem Bay evaluation module board.

Kindly help to review this patch set.

Thank you.

Changes since v3:
- Add Dt bindings for uhs gpio.
- Fixed comment by Michal and Sudeep on header file for the macro and error code.
- Fixed comment by Andy and created 1 new patch to separate the struc dev pointer in probe func.
- Fixed comment by Michal in arasan controller code.

Changes since v2:
- Removed Document DT Bindings for Keembay Firmware.
- Removed Firmware Driver to handle ATF Service call.
- Add header file to handle API function for device driver to communicate with Arm Trusted Firmware.

Changes since v1:
- Add Document DT Bindings for Keembay Firmware.
- Created Firmware Driver to handle ATF Service call
- Provide API for arasan driver for sd card voltage changes

Muhammad Husaini Zulkifli (4):
  firmware: keembay: Add support for Arm Trusted Firmware Service call
  dt-bindings: mmc: Add uhs-gpio for Keem Bay UHS-1 Support
  mmc: sdhci-of-arasan: Add structure device pointer in probe
  mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

 .../devicetree/bindings/mmc/arasan,sdhci.yaml |   8 +-
 drivers/mmc/host/sdhci-of-arasan.c            | 127 ++++++++++++++++++
 .../linux/firmware/intel/keembay_firmware.h   |  47 +++++++
 3 files changed, 181 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/firmware/intel/keembay_firmware.h

--
2.17.1

Comments

Michal Simek Oct. 8, 2020, 7:35 a.m. UTC | #1
On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> 
> Add header file to handle API function for device driver to communicate
> with Arm Trusted Firmware.
> 
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>  .../linux/firmware/intel/keembay_firmware.h   | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 include/linux/firmware/intel/keembay_firmware.h
> 
> diff --git a/include/linux/firmware/intel/keembay_firmware.h b/include/linux/firmware/intel/keembay_firmware.h
> new file mode 100644
> index 000000000000..8a62abcdfead
> --- /dev/null
> +++ b/include/linux/firmware/intel/keembay_firmware.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Intel Keembay SOC Firmware API Layer
> + *
> + *  Copyright (C) 2020-2021, Intel Corporation
> + *
> + *  Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@intel.com>
> + */
> +
> +#ifndef __FIRMWARE_KEEMBAY_SMC_H__
> +#define __FIRMWARE_KEEMBAY_SMC_H__
> +
> +#include <linux/arm-smccc.h>
> +
> +/*
> + * This file defines API function that can be called by device driver in order to
> + * communicate with Arm Trusted Firmware.
> + */
> +
> +/* Setting for Keem Bay IO Pad Line Voltage Selection */
> +#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,		\
> +			   ARM_SMCCC_SMC_32,		\
> +			   ARM_SMCCC_OWNER_SIP,		\
> +			   0xFF26)
> +
> +#define KEEMBAY_SET_1V8_VOLT	1
> +#define KEEMBAY_SET_3V3_VOLT	0
> +
> +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
> +static inline int keembay_sd_voltage_selection(int volt)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, &res);
> +	if ((int)res.a0 < 0)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +#else
> +static inline int keembay_sd_voltage_selection(int volt)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
> 

This looks good to me.

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal
Sudeep Holla Oct. 8, 2020, 9:45 a.m. UTC | #2
On Thu, Oct 08, 2020 at 10:09:33AM +0800, muhammad.husaini.zulkifli@intel.com wrote:
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>
> Add header file to handle API function for device driver to communicate
> with Arm Trusted Firmware.

[nit] Since it moved to trusted-firmware.org, it is no longer "Arm"
Trusted Firmware. It is now called Trusted Firmware - A profile(TF-A)
or Trusted Firmware - M profile (TF-M). Please update the subject and
the text above. I know it is silly but I am being asked to get this
fixed as it may create "confusion"(I don't know details, please don't
ask 😁)

Apart from various minor things Andy already pointed out, this looks
good. You can add by Ack once the above naming and all things pointed
by Andy are fixed.

--
Regards,
Sudeep
Zulkifli, Muhammad Husaini Oct. 8, 2020, 10:36 a.m. UTC | #3
Hi Michal,

Thanks for the comment. I replied inline

>-----Original Message-----

>From: Michal Simek <michal.simek@xilinx.com>

>Sent: Thursday, October 8, 2020 3:35 PM

>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>;

>Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;

>Shevchenko, Andriy <andriy.shevchenko@intel.com>; ulf.hansson@linaro.org;

>linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-

>kernel@vger.kernel.org

>Cc: Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>;

>Wan Mohamad, Wan Ahmad Zainie

><wan.ahmad.zainie.wan.mohamad@intel.com>; arnd@arndb.de

>Subject: Re: [PATCH v4 3/4] mmc: sdhci-of-arasan: Add structure device pointer

>in probe

>

>

>

>On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:

>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

>>

>> Add struct device *dev in probe func() so that it can widely use in

>> probe to make code more readable.

>>

>> Signed-off-by: Muhammad Husaini Zulkifli

>> <muhammad.husaini.zulkifli@intel.com>

>> ---

>>  drivers/mmc/host/sdhci-of-arasan.c | 1 +

>>  1 file changed, 1 insertion(+)

>>

>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c

>> b/drivers/mmc/host/sdhci-of-arasan.c

>> index f186fbd016b1..46aea6516133 100644

>> --- a/drivers/mmc/host/sdhci-of-arasan.c

>> +++ b/drivers/mmc/host/sdhci-of-arasan.c

>> @@ -1521,6 +1521,7 @@ static int sdhci_arasan_probe(struct

>platform_device *pdev)

>>  	struct sdhci_pltfm_host *pltfm_host;

>>  	struct sdhci_arasan_data *sdhci_arasan;

>>  	struct device_node *np = pdev->dev.of_node;

>> +	struct device *dev = &pdev->dev;

>>  	const struct sdhci_arasan_of_data *data;

>>

>>  	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);

>>

>

>This is not what we discussed. You create new variable and you should just use it

>in that function.

>

>s/pdev->dev\./dev->/g


For widely used in future, we plan to put it here and not specific to Keembay function only.
Any comment on this @Andy Shevchenko?
Thanks
>

>Thanks,

>Michal
Andy Shevchenko Oct. 8, 2020, 11:12 a.m. UTC | #4
On Thu, Oct 8, 2020 at 1:36 PM Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:
> >From: Michal Simek <michal.simek@xilinx.com>
> >Sent: Thursday, October 8, 2020 3:35 PM
> >On 08. 10. 20 4:09, muhammad.husaini.zulkifli@intel.com wrote:
> >> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

...

> >> @@ -1521,6 +1521,7 @@ static int sdhci_arasan_probe(struct
> >platform_device *pdev)
> >>      struct sdhci_pltfm_host *pltfm_host;
> >>      struct sdhci_arasan_data *sdhci_arasan;
> >>      struct device_node *np = pdev->dev.of_node;
> >> +    struct device *dev = &pdev->dev;
> >>      const struct sdhci_arasan_of_data *data;
> >>
> >>      match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
> >>
> >
> >This is not what we discussed. You create new variable and you should just use it
> >in that function.
> >
> >s/pdev->dev\./dev->/g
>
> For widely used in future, we plan to put it here and not specific to Keembay function only.
> Any comment on this @Andy Shevchenko?

I'm not sure what comment from me is needed. I'm on the same page with
Michal, i.e. replace current users of &pdev->dev with a new temporary
variable.
Zulkifli, Muhammad Husaini Oct. 8, 2020, 12:34 p.m. UTC | #5
Hi Sudeep,

>-----Original Message-----

>From: Sudeep Holla <sudeep.holla@arm.com>

>Sent: Thursday, October 8, 2020 5:45 PM

>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>

>Cc: Hunter, Adrian <adrian.hunter@intel.com>; michal.simek@xilinx.com;

>Shevchenko, Andriy <andriy.shevchenko@intel.com>; ulf.hansson@linaro.org;

>linux-mmc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-

>kernel@vger.kernel.org; Raja Subramanian, Lakshmi Bai

><lakshmi.bai.raja.subramanian@intel.com>; arnd@arndb.de; Wan Mohamad,

>Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>

>Subject: Re: [PATCH v4 1/4] firmware: keembay: Add support for Arm Trusted

>Firmware Service call

>

>On Thu, Oct 08, 2020 at 10:09:33AM +0800,

>muhammad.husaini.zulkifli@intel.com wrote:

>> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>

>>

>> Add header file to handle API function for device driver to

>> communicate with Arm Trusted Firmware.

>

>[nit] Since it moved to trusted-firmware.org, it is no longer "Arm"

>Trusted Firmware. It is now called Trusted Firmware - A profile(TF-A) or

>Trusted Firmware - M profile (TF-M). Please update the subject and the text

>above. I know it is silly but I am being asked to get this fixed as it may create

>"confusion"(I don't know details, please don't ask 😁)

>

>Apart from various minor things Andy already pointed out, this looks good.

>You can add by Ack once the above naming and all things pointed by Andy are

>fixed.


Noted. Thanks Sudeep 😊
>

>--

>Regards,

>Sudeep