Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH V2 2/2] mmc: mmci: initialize pwr|clk|datactrl_reg with
 their hardware values
To:     Ulf Hansson <ulf.hansson@linaro.org>
CC:     Rob Herring <robh+dt@kernel.org>,
        Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Alexandre Torgue <alexandre.torgue@st.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        DTML <devicetree@vger.kernel.org>,
        "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>
References: <20200325143409.13005-1-ludovic.barre@st.com>
 <20200325143409.13005-3-ludovic.barre@st.com>
 <CAPDyKFpO40Ois8pNwYOeqZrJbV9UeCLok=i8z4GWML+A7TP2-w@mail.gmail.com>
From:   Ludovic BARRE <ludovic.barre@st.com>
Message-ID: <734ef31d-5104-c7d4-bf14-6bb116484010@st.com>
Date:   Wed, 1 Apr 2020 16:56:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <CAPDyKFpO40Ois8pNwYOeqZrJbV9UeCLok=i8z4GWML+A7TP2-w@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

hi Ulf

Le 3/26/20 à 3:27 PM, Ulf Hansson a écrit :
> On Wed, 25 Mar 2020 at 15:34, Ludovic Barre <ludovic.barre@st.com> wrote:
>>
>> In mmci_write_pwr|clk|datactrlreg functions, if the desired value
>> is equal to corresponding variable (pwr_reg|clk_reg|datactrl_reg),
>> the value is not written in the register.
>>
>> At probe pwr|clk|datactrl_reg of mmci_host structure are initialized
>> to 0 (kzalloc of mmc_alloc_host). But they does not necessarily reflect
>> hardware value of these registers, if they are used while boot level.
>> This is problematic, if we want to write 0 in these registers.
> 
> It could be, but I don't see that we ever needs to do that - at least
> not before we have written some other non-zero values to them (through
> the helper functions).
> 

The sdmmc variant is slightly different on pwr_ctrl field
of MMCIPOWER register.

In classic mmci we have 3 or 2 values:
MMCI_PWR_OFF(0x0), MMCI_PWR_UP(0x2)optional, MMCI_PWR_ON(0x3)
-When you switch the external power supply off, the software set
  power-off (0x0).

-When you switch the external power supply on, the software first enters
  the power-up(0x2) phase, and waits until the supply output is stable
  before moving to the power-on (0x3)phase.

On sdmmc we have 3 values:
MMCI_PWR_OFF(0x0), MCI_STM32_PWR_CYC(0x2), MMCI_PWR_ON(0x3)
-When you switch the external power supply off, the software must
  MCI_STM32_PWR_CYC(0x2) => This will make that the SDMMC_D[7:0],
  SDMMC_CMD and SDMMC_CK are driven low, to prevent the card from being
  supplied through the signal lines.

-When you switch the external power supply on, when supply output is
  stable the MMCI_PWR_OFF(0x0) state can be apply (minimum 1ms) => The
  SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are set to drive “1”. After
  MMCI_PWR_ON(0x3) stat could be set.

In fact the last value of power off sequence is different between
classic and sdmmc:
The classic mmci finish the power off sequence by 0x0, But the sdmmc
finish by 0x2, and we must write 0x0 in power on sequence before set
MMCI_PWR_ON.
The 0x0 value is not written due to kzalloc value of pwr_reg
(which not reflect hardware value of pwr register).

>>
>> This patch initializes pwr|clk|datactrl_reg variables with their
>> hardware values while probing.
> 
> Hmm, so in principle this change seems quite okay, but I am hesitant
> to pick it up - unless it really addresses a problem that you have
> observed.
> 
> The reason is, this change may lead to avoiding to re-write the
> register with the same value it got during boot - and who knows what
> is best here.

I understand your hesitation.
If you prefer, I can move the pwr_reg initialisation
in sdmmc_variant_init ?

Regards
Ludo

> 
> Kind regards
> Uffe
> 
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> ---
>>   drivers/mmc/host/mmci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 647567def612..f378ae18d5dc 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -2085,6 +2085,10 @@ static int mmci_probe(struct amba_device *dev,
>>          else if (plat->ocr_mask)
>>                  dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n");
>>
>> +       host->pwr_reg = readl_relaxed(host->base + MMCIPOWER);
>> +       host->clk_reg = readl_relaxed(host->base + MMCICLOCK);
>> +       host->datactrl_reg = readl_relaxed(host->base + MMCIDATACTRL);
>> +
>>          /* We support these capabilities. */
>>          mmc->caps |= MMC_CAP_CMD23;
>>
>> --
>> 2.17.1
>>