Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201202150205.20150-1-muhammad.husaini.zulkifli@intel.com> <20201202150205.20150-5-muhammad.husaini.zulkifli@intel.com>
In-Reply-To: <20201202150205.20150-5-muhammad.husaini.zulkifli@intel.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Wed, 2 Dec 2020 19:54:55 +0100
Message-ID: <CACRpkdZznKd4NYk8whBtq1sUAj9uhasn3+ykrh50A2XKokp=Aw@mail.gmail.com>
Subject: Re: [PATCH v6 4/4] mmc: sdhci-of-arasan: Enable UHS-1 support for
 Keem Bay SOC
To:     muhammad.husaini.zulkifli@intel.com
Cc:     Ulf Hansson <ulf.hansson@linaro.org>,
        Adrian Hunter <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" <linux-kernel@vger.kernel.org>,
        Andriy Shevchenko <andriy.shevchenko@intel.com>,
        lakshmi.bai.raja.subramanian@intel.com,
        wan.ahmad.zainie.wan.mohamad@intel.com,
        Mark Gross <mgross@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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, &r=
es);
+       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 G=
PIO(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 =E2=82=AC0.01...

Yours,
Linus Walleij