Message-ID: <DM6PR11MB28767ED32E97BF93C5F3C7B4B8F20@DM6PR11MB2876.namprd11.prod.outlook.com>
Date:   Thu, 3 Dec 2020 07:10:14 +0000
From:   "Zulkifli, Muhammad Husaini" <muhammad.husaini.zulkifli@...el.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Ulf Hansson <ulf.hansson@...aro.org>,
        "Hunter, Adrian" <adrian.hunter@...el.com>,
        Michal Simek <michal.simek@...inx.com>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Shevchenko, Andriy" <andriy.shevchenko@...el.com>,
        "Raja Subramanian, Lakshmi Bai" 
        <lakshmi.bai.raja.subramanian@...el.com>,
        "Wan Mohamad, Wan Ahmad Zainie" 
        <wan.ahmad.zainie.wan.mohamad@...el.com>,
        Mark Gross <mgross@...ux.intel.com>
Subject: RE: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
 Keem Bay SOC

Hi Linus,

Thanks for your input. I replied inline.

>-----Original Message-----
>From: Linus Walleij <linus.walleij@...aro.org>
>Sent: Thursday, December 3, 2020 2:55 AM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@...el.com>
>Cc: Ulf Hansson <ulf.hansson@...aro.org>; Hunter, Adrian
><adrian.hunter@...el.com>; Michal Simek <michal.simek@...inx.com>; linux-
>mmc <linux-mmc@...r.kernel.org>; Linux ARM <linux-arm-
>kernel@...ts.infradead.org>; linux-kernel@...r.kernel.org; Shevchenko,
>Andriy <andriy.shevchenko@...el.com>; Raja Subramanian, Lakshmi Bai
><lakshmi.bai.raja.subramanian@...el.com>; Wan Mohamad, Wan Ahmad
>Zainie <wan.ahmad.zainie.wan.mohamad@...el.com>; Mark Gross
><mgross@...ux.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@...el.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

Powered by blists - more mailing lists