| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+V-a8v5VcBRjp-kPGp2pKXZ2RhCSXHdsL9X5YDOxjL6W1Mg=Q@mail.gmail.com>
Date: Thu, 4 Jul 2024 16:56:56 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Ulf Hansson <ulf.hansson@...aro.org>,
Rob Herring <robh@...nel.org>, Geert Uytterhoeven <geert+renesas@...der.be>,
Prabhakar <prabhakar.csengg@...il.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>, Biju Das <biju.das.jz@...renesas.com>,
linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-mmc@...r.kernel.org,
Magnus Damm <magnus.damm@...il.com>, Conor Dooley <conor+dt@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>
Subject: Re: [PATCH v4 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC
Hi Wolfram,
Thank you for the review.
On Wed, Jul 3, 2024 at 10:43 AM Wolfram Sang
<wsa+renesas@...g-engineering.com> wrote:
>
> Hi Prabhakar,
>
> On Wed, Jun 26, 2024 at 02:23:41PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > The SDHI/eMMC IPs found in the RZ/V2H(P) (a.k.a. r9a09g057) are very
> > similar to those found in R-Car Gen3. However, they are not identical,
> > necessitating an SoC-specific compatible string for fine-tuning driver
> > support.
> >
> > Key features of the RZ/V2H(P) SDHI/eMMC IPs include:
> > - Voltage level control via the IOVS bit.
> > - PWEN pin support via SD_STATUS register.
> > - Lack of HS400 support.
> > - Fixed address mode operation.
> >
> > internal regulator support is added to control the voltage levels of SD
> > pins via sd_iovs/sd_pwen bits in SD_STATUS register.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com> # on RZ/G3S
> > ---
> > v3->v4
> > - Dropped using 'renesas,sdhi-use-internal-regulator' property
> > - Now using of_device_is_available() to check if regulator is available and enabled
> > - Dropped extra spaces during operations
> > - Included tested by tag from Claudiu
> > - Rebased patch on top of https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240626085015.32171-2-wsa+renesas@sang-engineering.com/
> >
> > v2->v3
> > - Moved regulator info to renesas_sdhi_of_data instead of quirks
> > - Added support to configure the init state of regulator
> > - Added function pointers to configure regulator
> > - Added REGULATOR_CHANGE_VOLTAGE mask
> >
> > v1->v2
> > - Now controlling PWEN bit get/set_voltage
> > ---
> > drivers/mmc/host/renesas_sdhi.h | 13 ++
> > drivers/mmc/host/renesas_sdhi_core.c | 98 ++++++++++++
> > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 147 ++++++++++++++++++
> > drivers/mmc/host/tmio_mmc.h | 5 +
> > 4 files changed, 263 insertions(+)
> >
> > diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
> > index f12a87442338..cd509e7142ba 100644
> > --- a/drivers/mmc/host/renesas_sdhi.h
> > +++ b/drivers/mmc/host/renesas_sdhi.h
> > @@ -11,6 +11,8 @@
> >
> > #include <linux/dmaengine.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> > #include <linux/workqueue.h>
> > #include "tmio_mmc.h"
> >
> > @@ -36,6 +38,12 @@ struct renesas_sdhi_of_data {
> > unsigned int max_blk_count;
> > unsigned short max_segs;
> > unsigned long sdhi_flags;
> > + struct regulator_desc *rdesc;
> > + struct regulator_init_data *reg_init_data;
> > + bool regulator_init_state;
> > + unsigned int regulator_init_voltage;
> > + int (*regulator_force_endis)(struct regulator_dev *rdev, bool enable);
> > + int (*regulator_force_voltage)(struct regulator_dev *rdev, unsigned int voltage);
>
> I am open for discussing this but maybe here only
>
> + struct renesas_sdhi_regulator *internal_regulator
>
> or something and create the new struct with the additions above?
>
> > + int (*regulator_force_endis)(struct regulator_dev *rdev, bool enable);
> > + int (*regulator_force_voltage)(struct regulator_dev *rdev, unsigned int voltage);
>
> Do we need these functions because the regulator framework cannot force
> these actions because it caches the old state? I wonder if we can avoid
> these functions...
>
Yes, for the voltage setting, it caches the values. However, for the
regulator enable/disable, we can use is_enabled(), which probes the
hardware.
The reset value for PWEN is 1. The regulator_force_endis() callback is
mainly added for a scenario where, consider a code flow where the
regulator is disabled (using regulator_disable()) and now we land in
the reset callback (i.e., renesas_sdhi_reset()). Here, after issuing
the reset, the PWEN value will be 1, but we need to restore it back.
Hence, this callback is necessary. Note that is_enabled() cannot be
used, as it probes the hardware when it switches states after a reset.
The reset value for IOVS is 3.3V. Below is the scenario for which
regulator_force_voltage() is added:
-----> Current value: 1.8V (cached by the regulator)
--------------> After reset:
------------------> Hardware has 3.3V, but the regulator core cache
still has 1.8V.
----------------------> When requested to switch to 1.8V from MMC
core: The regulator core returns success, as it sees 1.8V in the
cached state.
----------------------------> As a result, the SD card won't work.
Cheers,
Prabhakar
Powered by blists - more mailing lists