| Message ID | 20200420161831.5043-1-ludovic.barre@st.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 33ba6fec0012e47f4e72bfab922b99327373f210 |
| Headers | show |
| Series | mmc: mmci_sdmmc: fix power on issue due to pwr_reg initialization | expand |
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? I am a little worried that we may start to rely on boot loader conditions, which isn't really what we want either... > > 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. Kind regards Uffe > --- > > This patch is the proposal from: > https://patchwork.kernel.org/patch/11457987/ > > --- > drivers/mmc/host/mmci_stm32_sdmmc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c > index d33e62bd6153..14f99d8aa3f0 100644 > --- a/drivers/mmc/host/mmci_stm32_sdmmc.c > +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c > @@ -519,6 +519,7 @@ void sdmmc_variant_init(struct mmci_host *host) > struct sdmmc_dlyb *dlyb; > > host->ops = &sdmmc_variant_ops; > + host->pwr_reg = readl_relaxed(host->base + MMCIPOWER); > > base_dlyb = devm_of_iomap(mmc_dev(host->mmc), np, 1, NULL); > if (IS_ERR(base_dlyb)) > -- > 2.17.1 >
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? > > I am a little worried that we may start to rely on boot loader > conditions, which isn't really what we want either... > > > > > 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? [...] Kind regards Uffe
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. >> >> 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). >>> >>> 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. Regards Ludo > > [...] > > Kind regards > Uffe >
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! Kind regards Uffe
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 >
diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c index d33e62bd6153..14f99d8aa3f0 100644 --- a/drivers/mmc/host/mmci_stm32_sdmmc.c +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c @@ -519,6 +519,7 @@ void sdmmc_variant_init(struct mmci_host *host) struct sdmmc_dlyb *dlyb; host->ops = &sdmmc_variant_ops; + host->pwr_reg = readl_relaxed(host->base + MMCIPOWER); base_dlyb = devm_of_iomap(mmc_dev(host->mmc), np, 1, NULL); if (IS_ERR(base_dlyb))
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". 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> --- This patch is the proposal from: https://patchwork.kernel.org/patch/11457987/ --- drivers/mmc/host/mmci_stm32_sdmmc.c | 1 + 1 file changed, 1 insertion(+)