Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Ulf Hansson]

On Wed, 2 Dec 2020 at 08:02, <muhammad.husaini.zulkifli@intel.com> wrote:
>
> From: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>
> Hello
>
> This patch series adds UHS-1 Support for Keem Bay SOC.
>
> Patch 1 : Add struct device *dev in probe func(),so that dev pointer can be widely use in probe to make code more readable.
> Patch 2 : Export function to be use by device driver to configure voltage rail output.
> Patch 3 : Defined the DT binding to support the UHS-1 operation.
> Patch 4 : Gives an overview of how voltage operations happen to support UHS-1 mode for Keem Bay SOC.
>
> All of these patches was tested with Keem Bay evaluation module board.
>
> Kindly help to review this patch set.

This version looks a lot better to me, but I am still requesting you
to model the pinctrl correctly. I don't see a reason not to, but I may
have overlooked some things.

Would you mind to re-submit to include the gpio/pinctlr list and the
maintainers, to get their opinion.

Kind regards
Uffe

>
> Thank you.
>
> Changes since v5:
> - Restructured voltage operation for io bus line in one function and added comments in sdhci-of-arasan.c.
> - Use gpio regulator modelling for expander pins.
> - Add DT Bindings for phys, vmmc and vqmmc supplies.
> - Renamed func name and added comments for firmware header files.
> - Update commit message for each patchs to describe what goes on.
>
> Changes since v4:
> - Fixed comment by Andy and Sudeep on first patch.
> - Add Acked-by tag.
> - Fixed comment by Michal and Andy on the third patch to replace current users of &pdev->dev.
>
> 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):
>   mmc: sdhci-of-arasan: Add structure device pointer in probe func
>   firmware: keembay: Add support for Trusted Firmware Service call
>   dt-bindings: mmc: Add phys, vmmc and vqmmc supplies for Keem Bay SOC
>   mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC
>
>  .../devicetree/bindings/mmc/arasan,sdhci.yaml |   6 +-
>  drivers/mmc/host/sdhci-of-arasan.c            | 282 ++++++++++++++++--
>  include/linux/firmware/intel/keembay.h        |  54 ++++
>  3 files changed, 322 insertions(+), 20 deletions(-)
>  create mode 100644 include/linux/firmware/intel/keembay.h
>
> --
> 2.17.1
>

Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Shevchenko, Andriy]

On Wed, Dec 02, 2020 at 11:53:42AM +0100, Ulf Hansson wrote:
> On Wed, 2 Dec 2020 at 08:02, <muhammad.husaini.zulkifli@intel.com> wrote:

...

> > Kindly help to review this patch set.
> 
> This version looks a lot better to me, but I am still requesting you
> to model the pinctrl correctly. I don't see a reason not to, but I may
> have overlooked some things.

I'm wondering why we need to mock up a pin control from something which has no
pin control interface. It's rather communication with firmware that does pin
control under the hood, but it also may be different hardware in the other /
future generations. Would you accept mocking up the same calls over the kernel
as pin control, as something else?

> Would you mind to re-submit to include the gpio/pinctlr list and the
> maintainers, to get their opinion.

And I will send immediately the same comment which I believe Linus W. supports.
But who knows...

Cc'ed to Linus as I mentioned him.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Ulf Hansson]

On Wed, 2 Dec 2020 at 13:24, Shevchenko, Andriy
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Dec 02, 2020 at 11:53:42AM +0100, Ulf Hansson wrote:
> > On Wed, 2 Dec 2020 at 08:02, <muhammad.husaini.zulkifli@intel.com> wrote:
>
> ...
>
> > > Kindly help to review this patch set.
> >
> > This version looks a lot better to me, but I am still requesting you
> > to model the pinctrl correctly. I don't see a reason not to, but I may
> > have overlooked some things.
>
> I'm wondering why we need to mock up a pin control from something which has no
> pin control interface. It's rather communication with firmware that does pin
> control under the hood, but it also may be different hardware in the other /
> future generations. Would you accept mocking up the same calls over the kernel
> as pin control, as something else?

Well, my point is that modeling this a pinctrl would keep the mmc
driver portable. Additionally, it's very common to manage pinctrls in
mmc drivers, so it's not like this is an entirely new thing that I
propose.

If/when it turns out that there is a new HW having a different pinctrl
interface, it would just mean that we need a new pinctrl driver, but
can leave the mmc driver as is.

>
> > Would you mind to re-submit to include the gpio/pinctlr list and the
> > maintainers, to get their opinion.
>
> And I will send immediately the same comment which I believe Linus W. supports.
> But who knows...
>
> Cc'ed to Linus as I mentioned him.

Thanks, let's see what Linus thinks then.

Kind regards
Uffe

Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Andy Shevchenko]

On Wed, Dec 2, 2020 at 2:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Wed, 2 Dec 2020 at 13:24, Shevchenko, Andriy
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Dec 02, 2020 at 11:53:42AM +0100, Ulf Hansson wrote:
> > > On Wed, 2 Dec 2020 at 08:02, <muhammad.husaini.zulkifli@intel.com> wrote:
> >
> > ...
> >
> > > > Kindly help to review this patch set.
> > >
> > > This version looks a lot better to me, but I am still requesting you
> > > to model the pinctrl correctly. I don't see a reason not to, but I may
> > > have overlooked some things.
> >
> > I'm wondering why we need to mock up a pin control from something which has no
> > pin control interface. It's rather communication with firmware that does pin
> > control under the hood, but it also may be different hardware in the other /
> > future generations. Would you accept mocking up the same calls over the kernel
> > as pin control, as something else?
>
> Well, my point is that modeling this a pinctrl would keep the mmc
> driver portable. Additionally, it's very common to manage pinctrls in
> mmc drivers, so it's not like this is an entirely new thing that I
> propose.
>
> If/when it turns out that there is a new HW having a different pinctrl
> interface, it would just mean that we need a new pinctrl driver, but
> can leave the mmc driver as is.

My point is that it may be *not* a pin control at all.
And in that case you will need to mock up (what exactly?) and update
the MMC driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Ulf Hansson]

On Wed, 2 Dec 2020 at 14:09, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Wed, Dec 2, 2020 at 2:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Wed, 2 Dec 2020 at 13:24, Shevchenko, Andriy
> > <andriy.shevchenko@intel.com> wrote:
> > > On Wed, Dec 02, 2020 at 11:53:42AM +0100, Ulf Hansson wrote:
> > > > On Wed, 2 Dec 2020 at 08:02, <muhammad.husaini.zulkifli@intel.com> wrote:
> > >
> > > ...
> > >
> > > > > Kindly help to review this patch set.
> > > >
> > > > This version looks a lot better to me, but I am still requesting you
> > > > to model the pinctrl correctly. I don't see a reason not to, but I may
> > > > have overlooked some things.
> > >
> > > I'm wondering why we need to mock up a pin control from something which has no
> > > pin control interface. It's rather communication with firmware that does pin
> > > control under the hood, but it also may be different hardware in the other /
> > > future generations. Would you accept mocking up the same calls over the kernel
> > > as pin control, as something else?
> >
> > Well, my point is that modeling this a pinctrl would keep the mmc
> > driver portable. Additionally, it's very common to manage pinctrls in
> > mmc drivers, so it's not like this is an entirely new thing that I
> > propose.
> >
> > If/when it turns out that there is a new HW having a different pinctrl
> > interface, it would just mean that we need a new pinctrl driver, but
> > can leave the mmc driver as is.
>
> My point is that it may be *not* a pin control at all.

Sorry, but I don't quite follow, what is *not* a pinctrl?

According to the information I have received from the previous
discussions [1], it's clear to me that the ARM SMC call ends up
changing settings for the I/O-pads. Or did I get that wrong?

> And in that case you will need to mock up (what exactly?) and update
> the MMC driver.
>
> --
> With Best Regards,
> Andy Shevchenko

Kind regards
Uffe

[1]
https://lkml.org/lkml/2020/10/8/320

Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Andy Shevchenko]

On Wed, Dec 2, 2020 at 4:10 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Wed, 2 Dec 2020 at 14:09, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Dec 2, 2020 at 2:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:

...

> > My point is that it may be *not* a pin control at all.
>
> Sorry, but I don't quite follow, what is *not* a pinctrl?
>
> According to the information I have received from the previous
> discussions [1], it's clear to me that the ARM SMC call ends up
> changing settings for the I/O-pads. Or did I get that wrong?

I'm discussing the possible implication of the solution (faking pin
control) you are proposing.
In this case we know that it's a pin control *under the hood of IPC*
(!) but in another hardware generation it may be, for example,custom
voltage regulator.

What you are proposing seems to me suboptimal and actually lying about
hardware. Because we do not have direct access to control this pad.
What we have is an IPC to firmware. And it's not our business what is
under the hood.

It seems it was a mistake to talk about these details in the first
place because it brings more confusion about hardware. So, consider
that it's not a pin control from OS perspective, but a firmware magic.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Zulkifli, Muhammad Husaini]

>-----Original Message-----
>From: Andy Shevchenko <andy.shevchenko@gmail.com>
>Sent: Wednesday, December 2, 2020 10:35 PM
>To: Ulf Hansson <ulf.hansson@linaro.org>
>Cc: Shevchenko, Andriy <andriy.shevchenko@intel.com>; Linus Walleij
><linus.walleij@linaro.org>; Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com>; Hunter, Adrian
><adrian.hunter@intel.com>; Michal Simek <michal.simek@xilinx.com>; linux-
>mmc@vger.kernel.org; Linux ARM <linux-arm-kernel@lists.infradead.org>;
>Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Raja Subramanian,
>Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad,
>Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Mark
>Gross <mgross@linux.intel.com>
>Subject: Re: [PATCH v6 0/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Wed, Dec 2, 2020 at 4:10 PM Ulf Hansson <ulf.hansson@linaro.org>
>wrote:
>> On Wed, 2 Dec 2020 at 14:09, Andy Shevchenko
><andy.shevchenko@gmail.com> wrote:
>> > On Wed, Dec 2, 2020 at 2:44 PM Ulf Hansson <ulf.hansson@linaro.org>
>wrote:
>
>...
>
>> > My point is that it may be *not* a pin control at all.
>>
>> Sorry, but I don't quite follow, what is *not* a pinctrl?
>>
>> According to the information I have received from the previous
>> discussions [1], it's clear to me that the ARM SMC call ends up
>> changing settings for the I/O-pads. Or did I get that wrong?
>
>I'm discussing the possible implication of the solution (faking pin
>control) you are proposing.
>In this case we know that it's a pin control *under the hood of IPC*
>(!) but in another hardware generation it may be, for example,custom voltage
>regulator.
>
>What you are proposing seems to me suboptimal and actually lying about
>hardware. Because we do not have direct access to control this pad.
>What we have is an IPC to firmware. And it's not our business what is under
>the hood.
>
>It seems it was a mistake to talk about these details in the first place because
>it brings more confusion about hardware. So, consider that it's not a pin
>control from OS perspective, but a firmware magic.

Maybe there is some misunderstanding regarding my statement in previous discussion.
I quoted "IO Pad" based on the statement in Databook CFG[1][10:7] for AON register.
From the Databook itself with additional confirmation from Keem Bay HW SOC Design Architect,
there is no direct control of these AON register bits from GPIO pads. 

>
>--
>With Best Regards,
>Andy Shevchenko

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

[muhammad.husaini.zulkifli]

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

Hello

This patch series adds UHS-1 Support for Keem Bay SOC.

Patch 1 : Add struct device *dev in probe func(),so that dev pointer can be widely use in probe to make code more readable.
Patch 2 : Export function to be use by device driver to configure voltage rail output.
Patch 3 : Defined the DT binding to support the UHS-1 operation.
Patch 4 : Gives an overview of how voltage operations happen to support UHS-1 mode for Keem Bay SOC.

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

Kindly help to review this patch set.

Thank you.

Changes since v5:
- Restructured voltage operation for io bus line in one function and added comments in sdhci-of-arasan.c.
- Use gpio regulator modelling for expander pins.
- Add DT Bindings for phys, vmmc and vqmmc supplies.
- Renamed func name and added comments for firmware header files.
- Update commit message for each patchs to describe what goes on.

Changes since v4:
- Fixed comment by Andy and Sudeep on first patch.
- Add Acked-by tag.
- Fixed comment by Michal and Andy on the third patch to replace current users of &pdev->dev.

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):
  mmc: sdhci-of-arasan: Add structure device pointer in probe func
  firmware: keembay: Add support for Trusted Firmware Service call
  dt-bindings: mmc: Add phys, vmmc and vqmmc supplies for Keem Bay SOC
  mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

 .../devicetree/bindings/mmc/arasan,sdhci.yaml |   6 +-
 drivers/mmc/host/sdhci-of-arasan.c            | 282 ++++++++++++++++--
 include/linux/firmware/intel/keembay.h        |  54 ++++
 3 files changed, 322 insertions(+), 20 deletions(-)
 create mode 100644 include/linux/firmware/intel/keembay.h

--
2.17.1

[PATCH v6 1/4] mmc: sdhci-of-arasan: Add structure device pointer in probe func

[muhammad.husaini.zulkifli]

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>
Reviewed-by: mark gross <mgross@linux.intel.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 38 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index d25a4b50c2f3..196e3d65277e 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -1508,9 +1508,10 @@ 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);
+	match = of_match_node(sdhci_arasan_of_match, dev->of_node);
 	data = match->data;
 	host = sdhci_pltfm_init(pdev, data->pdata, sizeof(*sdhci_arasan));
 
@@ -1524,42 +1525,42 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	sdhci_arasan->soc_ctl_map = data->soc_ctl_map;
 	sdhci_arasan->clk_ops = data->clk_ops;
 
-	node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
+	node = of_parse_phandle(dev->of_node, "arasan,soc-ctl-syscon", 0);
 	if (node) {
 		sdhci_arasan->soc_ctl_base = syscon_node_to_regmap(node);
 		of_node_put(node);
 
 		if (IS_ERR(sdhci_arasan->soc_ctl_base)) {
-			ret = dev_err_probe(&pdev->dev,
+			ret = dev_err_probe(dev,
 					    PTR_ERR(sdhci_arasan->soc_ctl_base),
 					    "Can't get syscon\n");
 			goto err_pltfm_free;
 		}
 	}
 
-	sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
+	sdhci_arasan->clk_ahb = devm_clk_get(dev, "clk_ahb");
 	if (IS_ERR(sdhci_arasan->clk_ahb)) {
-		dev_err(&pdev->dev, "clk_ahb clock not found.\n");
+		dev_err(dev, "clk_ahb clock not found.\n");
 		ret = PTR_ERR(sdhci_arasan->clk_ahb);
 		goto err_pltfm_free;
 	}
 
-	clk_xin = devm_clk_get(&pdev->dev, "clk_xin");
+	clk_xin = devm_clk_get(dev, "clk_xin");
 	if (IS_ERR(clk_xin)) {
-		dev_err(&pdev->dev, "clk_xin clock not found.\n");
+		dev_err(dev, "clk_xin clock not found.\n");
 		ret = PTR_ERR(clk_xin);
 		goto err_pltfm_free;
 	}
 
 	ret = clk_prepare_enable(sdhci_arasan->clk_ahb);
 	if (ret) {
-		dev_err(&pdev->dev, "Unable to enable AHB clock.\n");
+		dev_err(dev, "Unable to enable AHB clock.\n");
 		goto err_pltfm_free;
 	}
 
 	ret = clk_prepare_enable(clk_xin);
 	if (ret) {
-		dev_err(&pdev->dev, "Unable to enable SD clock.\n");
+		dev_err(dev, "Unable to enable SD clock.\n");
 		goto clk_dis_ahb;
 	}
 
@@ -1573,7 +1574,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 	pltfm_host->clk = clk_xin;
 
-	if (of_device_is_compatible(pdev->dev.of_node,
+	if (of_device_is_compatible(dev->of_node,
 				    "rockchip,rk3399-sdhci-5.1"))
 		sdhci_arasan_update_clockmultiplier(host, 0x0);
 
@@ -1588,7 +1589,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 	sdhci_arasan_update_baseclkfreq(host);
 
-	ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
+	ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, dev);
 	if (ret)
 		goto clk_disable_all;
 
@@ -1597,29 +1598,28 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 			arasan_zynqmp_execute_tuning;
 	}
 
-	arasan_dt_parse_clk_phases(&pdev->dev, &sdhci_arasan->clk_data);
+	arasan_dt_parse_clk_phases(dev, &sdhci_arasan->clk_data);
 
 	ret = mmc_of_parse(host->mmc);
 	if (ret) {
 		if (ret != -EPROBE_DEFER)
-			dev_err(&pdev->dev, "parsing dt failed (%d)\n", ret);
+			dev_err(dev, "parsing dt failed (%d)\n", ret);
 		goto unreg_clk;
 	}
 
 	sdhci_arasan->phy = ERR_PTR(-ENODEV);
-	if (of_device_is_compatible(pdev->dev.of_node,
+	if (of_device_is_compatible(dev->of_node,
 				    "arasan,sdhci-5.1")) {
-		sdhci_arasan->phy = devm_phy_get(&pdev->dev,
-						 "phy_arasan");
+		sdhci_arasan->phy = devm_phy_get(dev, "phy_arasan");
 		if (IS_ERR(sdhci_arasan->phy)) {
 			ret = PTR_ERR(sdhci_arasan->phy);
-			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
+			dev_err(dev, "No phy for arasan,sdhci-5.1.\n");
 			goto unreg_clk;
 		}
 
 		ret = phy_init(sdhci_arasan->phy);
 		if (ret < 0) {
-			dev_err(&pdev->dev, "phy_init err.\n");
+			dev_err(dev, "phy_init err.\n");
 			goto unreg_clk;
 		}
 
@@ -1644,7 +1644,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	if (!IS_ERR(sdhci_arasan->phy))
 		phy_exit(sdhci_arasan->phy);
 unreg_clk:
-	sdhci_arasan_unregister_sdclk(&pdev->dev);
+	sdhci_arasan_unregister_sdclk(dev);
 clk_disable_all:
 	clk_disable_unprepare(clk_xin);
 clk_dis_ahb:
-- 
2.17.1

[PATCH v6 2/4] firmware: keembay: Add support for Trusted Firmware Service call

[muhammad.husaini.zulkifli]

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

Export inline function to encapsulate AON_CFG1 for controling the
I/O Rail supplied voltage levels which communicate with Trusted Firmware.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/linux/firmware/intel/keembay.h | 54 ++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 include/linux/firmware/intel/keembay.h

diff --git a/include/linux/firmware/intel/keembay.h b/include/linux/firmware/intel/keembay.h
new file mode 100644
index 000000000000..b5e3e490807e
--- /dev/null
+++ b/include/linux/firmware/intel/keembay.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Intel Keembay SOC Firmware API Layer
+ *
+ *  Copyright (C) 2020, Intel Corporation
+ *
+ *  Author: 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 an API function that can be called by a device driver in order to
+ * communicate with Trusted Firmware - A profile(TF-A) or Trusted Firmware - M profile (TF-M).
+ */
+
+#define KEEMBAY_SET_1V8_IO_RAIL	1
+#define KEEMBAY_SET_3V3_IO_RAIL	0
+
+#define IOV_1V8			1800000
+#define IOV_3V3			3300000
+
+#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)
+
+#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
+/*
+ * Voltage applied on the IO Rail is controlled from the Always On Register using specific
+ * bits in AON_CGF1 register. This is a secure register. Keem Bay SOC cannot exposed this
+ * register address to the outside world.
+ */
+static inline int keembay_io_rail_supplied_voltage(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_io_rail_supplied_voltage(int volt)
+{
+	return -ENODEV;
+}
+#endif
+#endif /* __FIRMWARE_KEEMBAY_SMC_H__ */
-- 
2.17.1

[PATCH v6 3/4] dt-bindings: mmc: Add phys, vmmc and vqmmc supplies for Keem Bay SOC

[muhammad.husaini.zulkifli]

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

Add DT bindings of vmmc and vqmmc supplies of regulator and phys for
the phandle of sd0_phy which contain additional property for
otap delay and sel_clk_buffer.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml b/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
index 0753289fba84..0cbfdd840889 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.yaml
@@ -83,7 +83,7 @@ properties:
       - const: intel,keembay-sdhci-5.1-sd       # Intel Keem Bay SD controller
         description:
           For this device it is strongly suggested to include
-          arasan,soc-ctl-syscon.
+          arasan,soc-ctl-syscon, phys, vmmc-supply and vqmmc-supply.
       - const: intel,keembay-sdhci-5.1-sdio     # Intel Keem Bay SDIO controller
         description:
           For this device it is strongly suggested to include
@@ -299,5 +299,9 @@ examples:
           clock-names = "clk_xin", "clk_ahb";
           clocks = <&scmi_clk KEEM_BAY_PSS_AUX_SD0>,
                    <&scmi_clk KEEM_BAY_PSS_SD0>;
+          phys = <&sd0_phy>;
+          phy-names = "phy_arasan";
           arasan,soc-ctl-syscon = <&sd0_phy_syscon>;
+          vmmc-supply = <&reg_sd0_vcc>;
+          vqmmc-supply = <&reg_sd0_vqcc>;
     };
-- 
2.17.1

[PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[muhammad.husaini.zulkifli]

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

Keem Bay SOC can support dual voltage operations for GPIO SD Pins to
either 1.8V or 3.3V for bus IO line power. In order to operate the GPIOs
line for Clk,Cmd and Data on Keem Bay Hardware, it is important to
configure the supplied voltage applied to their I/O Rail and the output
of the i2c expander pin. Final Voltage applied on the GPIOs Line are
dependent by both supplied voltage rail and expander pin output as it is
been set after passing through the voltage sense resistor.

The Keem Bay HW is somewhat unique in the way of how IO bus line voltage
are been controlled. Output of the Expander pins is been configured using
regulator. Voltage rail output is being configured using
keembay_io_rail_supplied_voltage() API in the sdhci driver directly.
Pin control based implementation becomes problematic to control the
voltage rail due to the base address of Always On Register is
different from the base address of GPIO(Pinctrl). Thus, there is
no way to control the I/O Rail using GPIO Pad configuration.
On the other hand, using ARM SMC (Secure Monitor Call) directly from
pin control driver for the sake of implement it as pin control model
is not a good approach.

This patch was tested with Keem Bay EVM.

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>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 244 +++++++++++++++++++++++++++++
 1 file changed, 244 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 196e3d65277e..00ecdf54128c 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -23,6 +23,7 @@
 #include <linux/regmap.h>
 #include <linux/of.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/firmware/intel/keembay.h>
 
 #include "cqhci.h"
 #include "sdhci-pltfm.h"
@@ -79,6 +80,8 @@ struct sdhci_arasan_soc_ctl_field {
  * @baseclkfreq:	Where to find corecfg_baseclkfreq
  * @clockmultiplier:	Where to find corecfg_clockmultiplier
  * @support64b:		Where to find SUPPORT64B bit
+ * @otap_delay:		Where to find otap_delay
+ * @sel_clk_buffer:	Where to find clock buffer delay
  * @hiword_update:	If true, use HIWORD_UPDATE to access the syscon
  *
  * It's up to the licensee of the Arsan IP block to make these available
@@ -89,6 +92,8 @@ struct sdhci_arasan_soc_ctl_map {
 	struct sdhci_arasan_soc_ctl_field	baseclkfreq;
 	struct sdhci_arasan_soc_ctl_field	clockmultiplier;
 	struct sdhci_arasan_soc_ctl_field	support64b;
+	struct sdhci_arasan_soc_ctl_field	otap_delay;
+	struct sdhci_arasan_soc_ctl_field	sel_clk_buffer;
 	bool					hiword_update;
 };
 
@@ -189,6 +194,8 @@ static const struct sdhci_arasan_soc_ctl_map intel_keembay_soc_ctl_map = {
 	.baseclkfreq = { .reg = 0x0, .width = 8, .shift = 14 },
 	.clockmultiplier = { .reg = 0x4, .width = 8, .shift = 14 },
 	.support64b = { .reg = 0x4, .width = 1, .shift = 24 },
+	.otap_delay = { .reg = 0x24, .width = 5, .shift = 23 },
+	.sel_clk_buffer = { .reg = 0x2c, .width = 3, .shift = 25 },
 	.hiword_update = false,
 };
 
@@ -364,6 +371,144 @@ static int sdhci_arasan_voltage_switch(struct mmc_host *mmc,
 	return -EINVAL;
 }
 
+/**
+ * sdhci_arasan_keembay_io_line_supply_operation - Supply for the bus IO line power
+ *
+ * @mmc:	Pointer to sdhci_host
+ * @ios:	Pointer to IO bus setting
+ *
+ * For Keem Bay HW, in order to operate the GPIOs line for Clk,Cmd and Data,
+ * it is important to configure the supplied voltage applied to their I/O Rail
+ * and the output of the i2c expander Pin.
+ *
+ * Note that to configure the voltage rail setting, specific bits in AON_CFG
+ * register must be set. While to configure the i2c expander pin output,
+ * gpio regulator modelling is been used to control the pin state.
+ *
+ * Always on Domain register having different base address from GPIO base address
+ * and it is a secure register. There is no way to control the I/O Voltage Rail
+ * from the GPIO Pad. SMC CALL is been used to set the bits in AON_CFG1 register.
+ *
+ * Final Voltage applied on the GPIOs Line are dependent by both supplied voltage
+ * I/O Rail and expander pin output as it is been set after passing through the
+ * voltage sense resistor.
+ *
+ * Return: 0 on success and error value on error
+ */
+static int sdhci_arasan_keembay_io_line_supply_operation(struct mmc_host *mmc,
+						  struct mmc_ios *ios)
+{
+	int ret, min_uV, max_uV, volt_rail;
+
+	if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
+		min_uV = IOV_1V8;
+		max_uV = IOV_1V8;
+		volt_rail = KEEMBAY_SET_1V8_IO_RAIL;
+	} else {
+		min_uV = IOV_3V3;
+		max_uV = IOV_3V3;
+		volt_rail = KEEMBAY_SET_3V3_IO_RAIL;
+	}
+
+	/* If no vqmmc supply then we can't change the voltage */
+	if (IS_ERR(mmc->supply.vqmmc))
+		return -EINVAL;
+
+	ret = regulator_set_voltage(mmc->supply.vqmmc, min_uV, max_uV);
+	if (ret)
+		return ret;
+
+	/*
+	 * This is like a final gatekeeper. Need to ensure changed voltage
+	 * is settled before and after turn on this bit.
+	 */
+	usleep_range(1000, 1100);
+
+	ret = keembay_io_rail_supplied_voltage(volt_rail);
+	if (ret)
+		return ret;
+
+	usleep_range(1000, 1100);
+
+	return 0;
+}
+
+static int sdhci_arasan_keembay_voltage_switch(struct mmc_host *mmc,
+				       struct mmc_ios *ios)
+{
+	struct sdhci_host *host = mmc_priv(mmc);
+	u16 ctrl_2, clk;
+	int ret;
+
+	switch (ios->signal_voltage) {
+	case MMC_SIGNAL_VOLTAGE_180:
+		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		clk &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+		clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		if (clk & SDHCI_CLOCK_CARD_EN)
+			return -EAGAIN;
+
+		sdhci_writeb(host, SDHCI_POWER_ON | SDHCI_POWER_180,
+				   SDHCI_POWER_CONTROL);
+
+		ret = sdhci_arasan_keembay_io_line_supply_operation(mmc, ios);
+		if (ret)
+			return ret;
+
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		ctrl_2 |= SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+
+		/* Sleep for 5ms to stabilize 1.8V regulator */
+		usleep_range(5000, 5500);
+
+		/* 1.8V regulator output should be stable within 5 ms */
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (!(ctrl_2 & SDHCI_CTRL_VDD_180))
+			return -EAGAIN;
+
+		clk  = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+		clk |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+		break;
+	case MMC_SIGNAL_VOLTAGE_330:
+		ret = sdhci_arasan_keembay_io_line_supply_operation(mmc, ios);
+		if (ret)
+			return ret;
+
+		/* Set 1.8V Signal Enable in the Host Control2 register to 0 */
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		ctrl_2 &= ~SDHCI_CTRL_VDD_180;
+		sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+
+		/* Sleep for 5ms to stabilize 3.3V regulator */
+		usleep_range(5000, 5500);
+
+		/* 3.3V regulator output should be stable within 5 ms */
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if (ctrl_2 & SDHCI_CTRL_VDD_180)
+			return -EAGAIN;
+
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sdhci_arasan_keembay_select_drive_strength(struct mmc_card *card,
+					unsigned int max_dtr, int host_drv,
+					int card_drv, int *drv_type)
+{
+	if (card->host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_180)
+		*drv_type = MMC_SET_DRIVER_TYPE_C;
+
+	return 0;
+}
+
 static const struct sdhci_ops sdhci_arasan_ops = {
 	.set_clock = sdhci_arasan_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -964,6 +1109,77 @@ static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host)
 	sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz);
 }
 
+/**
+ * sdhci_arasan_update_otap_delay - Set otap delay
+ *
+ * This is used to manualy control the txclk Tap Delay,
+ * for flopping the final stage flops.
+ *
+ * NOTES:
+ * - Many existing devices don't seem to do this and work fine.  To keep
+ *   compatibility for old hardware where the device tree doesn't provide a
+ *   register map, this function is a noop if a soc_ctl_map hasn't been provided
+ *   for this platform.
+ *
+ * @host:		The sdhci_host
+ */
+static void sdhci_arasan_update_otap_delay(struct sdhci_host *host,
+						u32 value)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_arasan_soc_ctl_map *soc_ctl_map =
+		sdhci_arasan->soc_ctl_map;
+
+	/* Having a map is optional */
+	if (!soc_ctl_map)
+		return;
+
+	/* If we have a map, we expect to have a syscon */
+	if (!sdhci_arasan->soc_ctl_base) {
+		pr_warn("%s: Have regmap, but no soc-ctl-syscon\n",
+			mmc_hostname(host->mmc));
+		return;
+	}
+
+	sdhci_arasan_syscon_write(host, &soc_ctl_map->otap_delay, value);
+}
+
+/**
+ * sdhci_arasan_update_clk_sel_buff - Clock buffer select
+ *
+ * This is used to delay the clock buffer
+ *
+ * NOTES:
+ * - Many existing devices don't seem to do this and work fine.  To keep
+ *   compatibility for old hardware where the device tree doesn't provide a
+ *   register map, this function is a noop if a soc_ctl_map hasn't been provided
+ *   for this platform.
+ *
+ * @host:		The sdhci_host
+ */
+static void sdhci_arasan_update_sel_clkbuf(struct sdhci_host *host,
+						u32 value)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_arasan_soc_ctl_map *soc_ctl_map =
+		sdhci_arasan->soc_ctl_map;
+
+	/* Having a map is optional */
+	if (!soc_ctl_map)
+		return;
+
+	/* If we have a map, we expect to have a syscon */
+	if (!sdhci_arasan->soc_ctl_base) {
+		pr_warn("%s: Have regmap, but no soc-ctl-syscon\n",
+			mmc_hostname(host->mmc));
+		return;
+	}
+
+	sdhci_arasan_syscon_write(host, &soc_ctl_map->sel_clk_buffer, value);
+}
+
 static void sdhci_arasan_set_clk_delays(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1587,6 +1803,34 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
 	}
 
+	if (of_device_is_compatible(np, "intel,keembay-sdhci-5.1-sd")) {
+		struct device_node *phys;
+		u32 otap_delay, sel_clk_buffer;
+
+		phys = of_parse_phandle(dev->of_node, "phys", 0);
+		if (!phys) {
+			dev_err(dev, "Can't get phys for sd0\n");
+			ret = -ENODEV;
+			goto err_pltfm_free;
+		}
+
+		of_property_read_u32(phys,
+			"intel,keembay-emmc-phy-otap-dly", &otap_delay);
+		of_property_read_u32(phys,
+			"intel,keembay-emmc-phy-sel-clkbuf", &sel_clk_buffer);
+
+		of_node_put(phys);
+
+		sdhci_arasan_update_otap_delay(host, otap_delay);
+		sdhci_arasan_update_sel_clkbuf(host, sel_clk_buffer);
+
+		host->mmc_host_ops.start_signal_voltage_switch =
+			sdhci_arasan_keembay_voltage_switch;
+
+		host->mmc_host_ops.select_drive_strength =
+			sdhci_arasan_keembay_select_drive_strength;
+	}
+
 	sdhci_arasan_update_baseclkfreq(host);
 
 	ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, dev);
-- 
2.17.1

Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Linus Walleij]

Hi Muhammad,

thanks for your patch!

On Wed, Dec 2, 2020 at 8:04 AM <muhammad.husaini.zulkifli@intel.com> wrote:

> Keem Bay SOC can support dual voltage operations for GPIO SD Pins to
> either 1.8V or 3.3V for bus IO line power. In order to operate the GPIOs
> line for Clk,Cmd and Data on Keem Bay Hardware, it is important to
> configure the supplied voltage applied to their I/O Rail and the output
> of the i2c expander pin. Final Voltage applied on the GPIOs Line are
> dependent by both supplied voltage rail and expander pin output as it is
> been set after passing through the voltage sense resistor.

I think I understand this part.

> The Keem Bay HW is somewhat unique in the way of how IO bus line voltage
> are been controlled. Output of the Expander pins is been configured using
> regulator.

That much is clear.

> Voltage rail output is being configured using
> keembay_io_rail_supplied_voltage() API in the sdhci driver directly.

And that is an SMC call like that:

+static inline int keembay_io_rail_supplied_voltage(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;

That can set the voltage by calling into the Arm secure world I guess?

> Pin control based implementation becomes problematic to control the
> voltage rail due to the base address of Always On Register is
> different fromThe driver does not have to be in the the base address of GPIO(Pinctrl). Thus, there is
> no way to control the I/O Rail using GPIO Pad configuration.

I don't see why this would be pin control related, and that is as
you point out leading to some confused discussions here.

We do have something like this generic pin config:

 * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
 *      supplies, the argument to this parameter (on a custom format) tells
 *      the driver which alternative power source to use.

But it's ... yeah. It usually has a very specific purpose of selecting
one of two available voltage rails inside the SoC. And it needs to
apply to one pin or pin group. Also it kind of implies that those
voltages are always on.

As you say:

> From the Databook itself with additional confirmation from
> Keem Bay HW SOC Design Architect,
> there is no direct control of these AON register bits from
> GPIO pads.

The keembay_io_rail_supplied_voltage() more resembles a
selector (choose one on a menu) voltage regulator to me
if anything.

> On the other hand, using ARM SMC (Secure Monitor Call) directly from
> pin control driver for the sake of implement it as pin control model
> is not a good approach.

Yeah it has to be called from somewhere, if you want an abstraction
to make the driver neutral to any machine, then use a
selector regulator. It can be placed
anywhere in the kernel as long as you can reference it.

The register is called (according to the code) AON_CGF1
(really? not AON_CFG1?) and the "ON" part in "AON"  makes
it sound like "analog ON" implying this is something that can be
turned on/off and configured into two voltages and it has been
wrapped in these custom SMCCs by a secure world developer
(right?)

If it should use any abstraction it should be a selector regulator
IMO and while that may seem overengineered it adds something
because regulators are used in  the MMC subsystem for vdd
and vqmmc because we are handling the OCR mask with that
and it can support any amount of present and future
voltages for signal levels with that as well. Any future changes
to how the different signal voltages are set or which voltages
exist can then be done in that regulator driver.

Just my €0.01...

Yours,
Linus Walleij

RE: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Zulkifli, Muhammad Husaini]

Hi Linus,

Thanks for your input. I replied inline.

>-----Original Message-----
>From: Linus Walleij <linus.walleij@linaro.org>
>Sent: Thursday, December 3, 2020 2:55 AM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Ulf Hansson <ulf.hansson@linaro.org>; Hunter, Adrian
><adrian.hunter@intel.com>; Michal Simek <michal.simek@xilinx.com>; linux-
>mmc <linux-mmc@vger.kernel.org>; Linux ARM <linux-arm-
>kernel@lists.infradead.org>; linux-kernel@vger.kernel.org; Shevchenko,
>Andriy <andriy.shevchenko@intel.com>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Mark Gross
><mgross@linux.intel.com>
>Subject: Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>Hi Muhammad,
>
>thanks for your patch!
>
>On Wed, Dec 2, 2020 at 8:04 AM <muhammad.husaini.zulkifli@intel.com>
>wrote:
>
>> Keem Bay SOC can support dual voltage operations for GPIO SD Pins to
>> either 1.8V or 3.3V for bus IO line power. In order to operate the
>> GPIOs line for Clk,Cmd and Data on Keem Bay Hardware, it is important
>> to configure the supplied voltage applied to their I/O Rail and the
>> output of the i2c expander pin. Final Voltage applied on the GPIOs
>> Line are dependent by both supplied voltage rail and expander pin
>> output as it is been set after passing through the voltage sense resistor.
>
>I think I understand this part.
>
>> The Keem Bay HW is somewhat unique in the way of how IO bus line
>> voltage are been controlled. Output of the Expander pins is been
>> configured using regulator.
>
>That much is clear.
>
>> Voltage rail output is being configured using
>> keembay_io_rail_supplied_voltage() API in the sdhci driver directly.
>
>And that is an SMC call like that:
>
>+static inline int keembay_io_rail_supplied_voltage(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;
>
>That can set the voltage by calling into the Arm secure world I guess?

Yes calling the Arm SMCC . 
The selection of the supply voltage applied to the rail depends on the AON_CFG1[9] .
Setting Bit9  will change the voltage rail supply.
Set Bit 9 = 1 // Operation voltage is 1.8V 
Set Bit 9 = 0 // Operation voltage is 3.3V

>
>> Pin control based implementation becomes problematic to control the
>> voltage rail due to the base address of Always On Register is
>> different fromThe driver does not have to be in the the base address
>> of GPIO(Pinctrl). Thus, there is no way to control the I/O Rail using GPIO
>Pad configuration.
>
>I don't see why this would be pin control related, and that is as you point out
>leading to some confused discussions here.

This is not related to a pin control. I pointed out this in the commit description to 
indicate that for Keem Bay HW to modelling as pinctrl to call the SMCC Arm is not 
a good approach.

>
>We do have something like this generic pin config:
>
> * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different
>power
> *      supplies, the argument to this parameter (on a custom format) tells
> *      the driver which alternative power source to use.
>
>But it's ... yeah. It usually has a very specific purpose of selecting one of two
>available voltage rails inside the SoC. And it needs to apply to one pin or pin
>group. Also it kind of implies that those voltages are always on.
>

For Keembay HW, we could not apply on any pins because we do not have a 
direct access to control this pad. That is why, currently I configure this voltage rail 
supply through the SMCCs Arm wrapper. 

Unless if we fake some pin example GPIO32-37 for SD, which using "*pinconf_set" pointed
to config param of PIN_CONFIG_POWER_SOURCE, then call the keembay_io_rail_supplied_voltage()
to change the IO rail voltage feeding.
By means calling keembay_io_rail_supplied_voltage(), we call ARM SMCC in this case condition ?
I was referring to this https://elixir.bootlin.com/linux/v5.10-rc6/source/drivers/pinctrl/renesas/pinctrl.c#L706 

>As you say:
>
>> From the Databook itself with additional confirmation from Keem Bay HW
>> SOC Design Architect, there is no direct control of these AON register
>> bits from GPIO pads.
>
>The keembay_io_rail_supplied_voltage() more resembles a selector (choose
>one on a menu) voltage regulator to me if anything.

Keem Bay HW SoC does not have its own voltage regulator for sd card.
Final IO line voltage depends on supplied voltage applied to their I/O 
Rail and the output of the i2c expander pin which go into power mux.

>
>> On the other hand, using ARM SMC (Secure Monitor Call) directly from
>> pin control driver for the sake of implement it as pin control model
>> is not a good approach.
>
>Yeah it has to be called from somewhere, if you want an abstraction to make
>the driver neutral to any machine, then use a selector regulator. It can be
>placed anywhere in the kernel as long as you can reference it.

Sorry. I am not really aware of selector regulator. Can you point me to any references
for this? 

>
>The register is called (according to the code) AON_CGF1 (really? not
>AON_CFG1?) and the "ON" part in "AON"  makes it sound like "analog ON"
>implying this is something that can be turned on/off and configured into two
>voltages and it has been wrapped in these custom SMCCs by a secure world
>developer
>(right?)

It is Always On Power Domain Register. Yes it's been wrapped with SMCCC call 
due to secure register. By default voltage rail feed is 3.3v.

>
>If it should use any abstraction it should be a selector regulator IMO and
>while that may seem overengineered it adds something because regulators
>are used in  the MMC subsystem for vdd and vqmmc because we are handling
>the OCR mask with that and it can support any amount of present and future
>voltages for signal levels with that as well. Any future changes to how the
>different signal voltages are set or which voltages exist can then be done in
>that regulator driver.

This is limitation of Keem Bay HW and I would say Keem Bay HW is somewhat 
unique in the way of handling the IO bus line voltage.
SDcard does not have its own voltage regulator.
I created one function sdhci_arasan_keembay_io_line_supply_operation() in sdhci-of-arasan.c  
to handle the vqmmc(io line supply operation) specific for Keem Bay SoC.

For Keem Bay, to actually modelling this as regulator ,for vqmmc, , we need to handle 2 things:
1) Output expander pins : using gpio regulator
2) voltage rail : call keembay_io_rail_supplied_voltage() to handle the SMCC Arm.

Other hardware might not need this as they might easily configure the vqmmc 
hooked up to regulator.
 
IMHO, we do not need to overengineered it to add custom selector 
regulator just to suit this Keem Bay HW design.

>
>Just my €0.01...
>
>Yours,
>Linus Walleij

Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Shevchenko, Andriy]

On Thu, Dec 03, 2020 at 09:10:14AM +0200, Zulkifli, Muhammad Husaini wrote:
> >From: Linus Walleij <linus.walleij@linaro.org>
> >Sent: Thursday, December 3, 2020 2:55 AM
> >On Wed, Dec 2, 2020 at 8:04 AM <muhammad.husaini.zulkifli@intel.com>
> >wrote:

...

> >If it should use any abstraction it should be a selector regulator IMO and
> >while that may seem overengineered it adds something because regulators
> >are used in  the MMC subsystem for vdd and vqmmc because we are handling
> >the OCR mask with that and it can support any amount of present and future
> >voltages for signal levels with that as well. Any future changes to how the
> >different signal voltages are set or which voltages exist can then be done in
> >that regulator driver.
> 
> This is limitation of Keem Bay HW and I would say Keem Bay HW is somewhat
> unique in the way of handling the IO bus line voltage.
> SDcard does not have its own voltage regulator.
> I created one function sdhci_arasan_keembay_io_line_supply_operation() in sdhci-of-arasan.c
> to handle the vqmmc(io line supply operation) specific for Keem Bay SoC.
> 
> For Keem Bay, to actually modelling this as regulator ,for vqmmc, , we need to handle 2 things:
> 1) Output expander pins : using gpio regulator
> 2) voltage rail : call keembay_io_rail_supplied_voltage() to handle the SMCC Arm.
> 
> Other hardware might not need this as they might easily configure the vqmmc
> hooked up to regulator.
> 
> IMHO, we do not need to overengineered it to add custom selector
> regulator just to suit this Keem Bay HW design.

I guess Linus has a point. If it can be abstracted as selector regulator it
will suits generic approach in the MMC code.

And what is the problem to have two or more regulators? Or regulator hierarchy?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Linus Walleij]

On Thu, Dec 3, 2020 at 8:10 AM Zulkifli, Muhammad Husaini
<muhammad.husaini.zulkifli@intel.com> wrote:

> >Yeah it has to be called from somewhere, if you want an abstraction to make
> >the driver neutral to any machine, then use a selector regulator. It can be
> >placed anywhere in the kernel as long as you can reference it.
>
> Sorry. I am not really aware of selector regulator. Can you point me to any references
> for this?

It is part of the regulator subsystem and the standard framework
there to handle regulators with an enumerable number of
specific voltage levels.

> IMHO, we do not need to overengineered it to add custom selector
> regulator just to suit this Keem Bay HW design.

That can be said about a lot of things we model with vqmmc.
Using standard abstractions makes things easier for maintainers.
We mostly design abstractions for maintenance not for the simplest way
to set bits in registers.

Yours,
Linus Walleij

RE: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for Keem Bay SOC

[Zulkifli, Muhammad Husaini]

Hi Linus,

>-----Original Message-----
>From: Linus Walleij <linus.walleij@linaro.org>
>Sent: Sunday, December 6, 2020 7:02 AM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: Ulf Hansson <ulf.hansson@linaro.org>; Hunter, Adrian
><adrian.hunter@intel.com>; Michal Simek <michal.simek@xilinx.com>; linux-
>mmc <linux-mmc@vger.kernel.org>; Linux ARM <linux-arm-
>kernel@lists.infradead.org>; linux-kernel@vger.kernel.org; Shevchenko,
>Andriy <andriy.shevchenko@intel.com>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@intel.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>; Mark Gross
><mgross@linux.intel.com>
>Subject: Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
>Keem Bay SOC
>
>On Thu, Dec 3, 2020 at 8:10 AM Zulkifli, Muhammad Husaini
><muhammad.husaini.zulkifli@intel.com> wrote:
>
>> >Yeah it has to be called from somewhere, if you want an abstraction
>> >to make the driver neutral to any machine, then use a selector
>> >regulator. It can be placed anywhere in the kernel as long as you can
>reference it.
>>
>> Sorry. I am not really aware of selector regulator. Can you point me
>> to any references for this?
>
>It is part of the regulator subsystem and the standard framework there to
>handle regulators with an enumerable number of specific voltage levels.
>
>> IMHO, we do not need to overengineered it to add custom selector
>> regulator just to suit this Keem Bay HW design.
>
>That can be said about a lot of things we model with vqmmc.
>Using standard abstractions makes things easier for maintainers.
>We mostly design abstractions for maintenance not for the simplest way to
>set bits in registers.

Thanks for your input. I will try model it similar to current regulator implementation
specific for keem bay SoC where this keem bay regulator will encapsulated the 
Secure Monitor Calling Convention (SMCCC) during voltage operations.

>
>Yours,
>Linus Walleij