Messages in this thread | | | Subject | Re: [PATCH] mmc: mmci_sdmmc: fix power on issue due to pwr_reg initialization | From | Ludovic BARRE <> | Date | Thu, 23 Apr 2020 10:12:03 +0200 |
| |
Le 4/22/20 à 6:03 PM, Ulf Hansson a écrit : > On Wed, 22 Apr 2020 at 15:40, Ludovic BARRE <ludovic.barre@st.com> wrote: >> >> hi Ulf >> >> Le 4/21/20 à 11:38 AM, Ulf Hansson a écrit : >>> On Tue, 21 Apr 2020 at 11:25, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> >>>> On Mon, 20 Apr 2020 at 18:18, Ludovic Barre <ludovic.barre@st.com> wrote: >>>>> >>>>> This patch fix a power-on issue, and avoid to retry the power sequence. >>>>> >>>>> In power off sequence: sdmmc must set pwr_reg in "power-cycle" state >>>>> (value 0x2), to prevent the card from being supplied through the signal >>>>> lines (all the lines are driven low). >>>>> >>>>> In power on sequence: when the power is stable, sdmmc must set pwr_reg >>>>> in "power-off" state (value 0x0) to drive all signal to high before to >>>>> set "power-on". >>>> >>>> Just a question to gain further understanding. >>>> >>>> Let's assume that the controller is a power-on state, because it's >>>> been initialized by the boot loader. When the mmc core then starts the >>>> power-on sequence (not doing a power-off first), would $subject patch >>>> then cause the >>>> MMCIPOWER to remain as is, or is it going to be overwritten? >> >> On sdmmc controller, the PWRCTRL[1:0] field of MMCIPOWER register allow >> to manage sd lines and has a specific bahavior. >> >> PWRCTRL value: >> - 0x0: After reset, Reset: the SDMMC is disabled and the clock to the >> Card is stopped, SDMMC_D[7:0], and SDMMC_CMD are HiZ and >> SDMMC_CK is driven low. >> When written 00, power-off: the SDMMC is disabled and the clock >> to the card is stopped, SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK >> are driven high. >> >> - 0x2: Power-cycle, the SDMMC is disabled and the clock to the card is >> stopped, SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are driven low. >> >> - 0x3: Power-on: the card is clocked, The first 74 SDMMC_CK cycles the >> SDMMC is still disabled. After the 74 cycles the SDMMC is >> enabled and the SDMMC_D[7:0], SDMMC_CMD and SDMMC_CK are >> controlled according the SDMMC operation. >> **Any further write will be ignored, PWRCTRL value >> will keep 0x3**. when the SDMMC is ON (0x3) only a reset could >> change pwrctrl value and the state of sdmmc lines. >> >> So if the lines are already "ON", the power-on sequence (decribed in >> commit message) not overwrite the pwctrl field and not disturb the sdmmc >> lines. > > Thanks for the detailed information, much appreciated! > >> >>>> >>>> I am a little worried that we may start to rely on boot loader >>>> conditions, which isn't really what we want either... >>>> >> >> We not depend of boot loader conditions. >> >> This patch simply allows to drive high the sd lines before to set >> "power-on" value (no effect if already power ON). > > Yep, thanks! > >> >>>>> >>>>> To avoid writing the same value to the power register several times, this >>>>> register is cached by the pwr_reg variable. At probe pwr_reg is initialized >>>>> to 0 by kzalloc of mmc_alloc_host. >>>>> >>>>> Like pwr_reg value is 0 at probing, the power on sequence fail because >>>>> the "power-off" state is not writes (value 0x0) and the lines >>>>> remain drive to low. >>>>> >>>>> This patch initializes "pwr_reg" variable with power register value. >>>>> This it done in sdmmc variant init to not disturb default mmci behavior. >>>>> >>>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >>>> >>>> Besides the comment, the code and the approach seems reasonable to me. >>> >>> Another related question. I just realized why you probably haven't set >>> .pwrreg_nopower for the variant_stm32_sdmmc and variant_stm32_sdmmcv2. >>> >>> I guess it's because you need a slightly different way to restore the >>> context of MMCIPOWER register at ->runtime_resume(), rather than just >>> re-writing it with the saved register values. Is this something that >>> you are looking into as well? >> >> Yes exactly, the sequence is slightly different. I can't write 0 on >> mmci_runtime_suspend, and can't just re-writing the saved register. > > So, it seems like you need to use the ->set_ios() callback, to > re-configure the controller correctly. > > Just tell if you need more help to make that work, otherwise I am here > to review your patches. > > In regards to $subject patch, I have applied it for next, thanks!
Thanks for your review. Have a nice day.
> > Kind regards > Uffe >
| |