| Message ID | 1551427028-103750-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mmc: core: Add MMC_CAP2_NO_TUNING_CMP support | expand |
On Fri, 1 Mar 2019 at 08:58, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > This caps2 is to skip doing memory compare tuning block > pattern with the received data from devices when doing > tuning. > > The data from device is all formatted like: > Start bit + data + CRC + End bit > > For normal data read, we just verify host sample the data > w/o any errors, but not need care the data context. But for > a tuning process, we now compare the data context with JEDEC > pattern. I highly suspect the rational behide it for the following > details: > > (1) The tuning process is mostly used for identifing the rational > sample window, so if the host read the tuning block successfully > like a noraml data read, isn't it enough for sure that *this* single > sample phase is good to work, no? The original patch to support it > didn't quote any spec details but let's see the JESD84-B51, section 6.6.5, > "Bus Sampling Tuning Concept" just says "This example is given for > concept explanation purpose. The Host may use any other implementation > according to its design consideration. " The concept isn't the order, > and whether the MMC core need to verify the tuning block? See my argue (2). > > (2) AFAICT, the tuning block is handled by a fixed HW module within > the device, if a JEDEC compatible device receive tuning block cmd and > response it, could it sends back a non-compatible tuning block? It's hard to tell whether the above makes sense. I think only the HW designers can give us a good answer on this, have you talked with someone that can confirm your analyze? > > (3) Some(it's impossible for me to check all) of the SDHCI IP instances > at hand which do tuning by HW block,never check the tuning block at all. I don't think that's correct, at least by looking at the result from: git grep -C 5 mmc_send_tuning To me it looks like it's only mtk-sd, that in some case only checks the cmd errors, but in others the error check also includes the memcmp with the tuning block. In other words, what you propose is something entirely new. > > I'm intended to remove it entirely but just stay back a bit for > skipping that if any vendors are willing to try(At least rockchip does). An option to the host cap, could be to return a special error code, like -EPROTO instead of -EIO as currently, in case the memcmp with the tuning block fails. This would allow the host to dynamically decide when it makes sense to check for the error code, and when it doesn't. Would that work for you? Kind regards Uffe > > Any comments will be appreciated. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/core/mmc_ops.c | 3 ++- > include/linux/mmc/host.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > index c5208fb..802cba5 100644 > --- a/drivers/mmc/core/mmc_ops.c > +++ b/drivers/mmc/core/mmc_ops.c > @@ -664,7 +664,8 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error) > goto out; > } > > - if (memcmp(data_buf, tuning_block_pattern, size)) > + if (!(host->caps & MMC_CAP2_NO_TUNING_CMP) && > + memcmp(data_buf, tuning_block_pattern, size)) > err = -EIO; > > out: > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 43d0f0c..8e759ff 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -370,6 +370,7 @@ struct mmc_host { > #define MMC_CAP2_CQE (1 << 23) /* Has eMMC command queue engine */ > #define MMC_CAP2_CQE_DCMD (1 << 24) /* CQE can issue a direct command */ > #define MMC_CAP2_AVOID_3_3V (1 << 25) /* Host must negotiate down from 3.3V */ > +#define MMC_CAP2_NO_TUNING_CMP (1 << 25) /* Do not need compare the tuning block */ > > int fixed_drv_type; /* fixed driver type for non-removable media */ > > -- > 1.9.1 > > >
Hi Ulf On 2019/3/5 17:31, Ulf Hansson wrote: > On Fri, 1 Mar 2019 at 08:58, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> >> This caps2 is to skip doing memory compare tuning block >> pattern with the received data from devices when doing >> tuning. >> >> The data from device is all formatted like: >> Start bit + data + CRC + End bit >> >> For normal data read, we just verify host sample the data >> w/o any errors, but not need care the data context. But for >> a tuning process, we now compare the data context with JEDEC >> pattern. I highly suspect the rational behide it for the following >> details: >> >> (1) The tuning process is mostly used for identifing the rational >> sample window, so if the host read the tuning block successfully >> like a noraml data read, isn't it enough for sure that *this* single >> sample phase is good to work, no? The original patch to support it >> didn't quote any spec details but let's see the JESD84-B51, section 6.6.5, >> "Bus Sampling Tuning Concept" just says "This example is given for >> concept explanation purpose. The Host may use any other implementation >> according to its design consideration. " The concept isn't the order, >> and whether the MMC core need to verify the tuning block? See my argue (2). >> >> (2) AFAICT, the tuning block is handled by a fixed HW module within >> the device, if a JEDEC compatible device receive tuning block cmd and >> response it, could it sends back a non-compatible tuning block? > > It's hard to tell whether the above makes sense. I think only the HW > designers can give us a good answer on this, have you talked with > someone that can confirm your analyze? > I haven't talked with HW designers from device vendors. >> >> (3) Some(it's impossible for me to check all) of the SDHCI IP instances >> at hand which do tuning by HW block,never check the tuning block at all. > > I don't think that's correct, at least by looking at the result from: > > git grep -C 5 mmc_send_tuning > > To me it looks like it's only mtk-sd, that in some case only checks > the cmd errors, but in others the error check also includes the memcmp > with the tuning block. > > In other words, what you propose is something entirely new. > >> >> I'm intended to remove it entirely but just stay back a bit for >> skipping that if any vendors are willing to try(At least rockchip does). > > An option to the host cap, could be to return a special error code, > like -EPROTO instead of -EIO as currently, in case the memcmp with the > tuning block fails. > > This would allow the host to dynamically decide when it makes sense to > check for the error code, and when it doesn't. > > Would that work for you? I don't get your point. What I mean is to skip doing memcmp but not checking whether or why memcmp failed after calling mmc_send_tuning(). The whole point is if the controller has the HW capability to check the tuning block, so data->error in mmc_send_tuning will let mmc core know if it need to manually compare the raw tuning block data. > > Kind regards > Uffe > >> >> Any comments will be appreciated. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/core/mmc_ops.c | 3 ++- >> include/linux/mmc/host.h | 1 + >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c >> index c5208fb..802cba5 100644 >> --- a/drivers/mmc/core/mmc_ops.c >> +++ b/drivers/mmc/core/mmc_ops.c >> @@ -664,7 +664,8 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error) >> goto out; >> } >> >> - if (memcmp(data_buf, tuning_block_pattern, size)) >> + if (!(host->caps & MMC_CAP2_NO_TUNING_CMP) && >> + memcmp(data_buf, tuning_block_pattern, size)) >> err = -EIO; >> >> out: >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 43d0f0c..8e759ff 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -370,6 +370,7 @@ struct mmc_host { >> #define MMC_CAP2_CQE (1 << 23) /* Has eMMC command queue engine */ >> #define MMC_CAP2_CQE_DCMD (1 << 24) /* CQE can issue a direct command */ >> #define MMC_CAP2_AVOID_3_3V (1 << 25) /* Host must negotiate down from 3.3V */ >> +#define MMC_CAP2_NO_TUNING_CMP (1 << 25) /* Do not need compare the tuning block */ >> >> int fixed_drv_type; /* fixed driver type for non-removable media */ >> >> -- >> 1.9.1 >> >> >> > >
On Wed, 6 Mar 2019 at 01:45, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > Hi Ulf > > On 2019/3/5 17:31, Ulf Hansson wrote: > > On Fri, 1 Mar 2019 at 08:58, Shawn Lin <shawn.lin@rock-chips.com> wrote: > >> > >> This caps2 is to skip doing memory compare tuning block > >> pattern with the received data from devices when doing > >> tuning. > >> > >> The data from device is all formatted like: > >> Start bit + data + CRC + End bit > >> > >> For normal data read, we just verify host sample the data > >> w/o any errors, but not need care the data context. But for > >> a tuning process, we now compare the data context with JEDEC > >> pattern. I highly suspect the rational behide it for the following > >> details: > >> > >> (1) The tuning process is mostly used for identifing the rational > >> sample window, so if the host read the tuning block successfully > >> like a noraml data read, isn't it enough for sure that *this* single > >> sample phase is good to work, no? The original patch to support it > >> didn't quote any spec details but let's see the JESD84-B51, section 6.6.5, > >> "Bus Sampling Tuning Concept" just says "This example is given for > >> concept explanation purpose. The Host may use any other implementation > >> according to its design consideration. " The concept isn't the order, > >> and whether the MMC core need to verify the tuning block? See my argue (2). > >> > >> (2) AFAICT, the tuning block is handled by a fixed HW module within > >> the device, if a JEDEC compatible device receive tuning block cmd and > >> response it, could it sends back a non-compatible tuning block? > > > > It's hard to tell whether the above makes sense. I think only the HW > > designers can give us a good answer on this, have you talked with > > someone that can confirm your analyze? > > > > I haven't talked with HW designers from device vendors. You don't need to card vendors, but your own HW guys that develops your mmc controller. But, I assume you have done that, just wanted to confirm it!? The point is, that you are referring to the JEDEC spec in the changelog, which in the end doesn't described the internal specific behavior of your mmc controller. Perhaps you could clarify that in the next version of the changelog. > > >> > >> (3) Some(it's impossible for me to check all) of the SDHCI IP instances > >> at hand which do tuning by HW block,never check the tuning block at all. > > > > I don't think that's correct, at least by looking at the result from: > > > > git grep -C 5 mmc_send_tuning > > > > To me it looks like it's only mtk-sd, that in some case only checks > > the cmd errors, but in others the error check also includes the memcmp > > with the tuning block. > > > > In other words, what you propose is something entirely new. > > > >> > >> I'm intended to remove it entirely but just stay back a bit for > >> skipping that if any vendors are willing to try(At least rockchip does). > > > > An option to the host cap, could be to return a special error code, > > like -EPROTO instead of -EIO as currently, in case the memcmp with the > > tuning block fails. > > > > This would allow the host to dynamically decide when it makes sense to > > check for the error code, and when it doesn't. > > > > Would that work for you? > > I don't get your point. What I mean is to skip doing memcmp but not > checking whether or why memcmp failed after calling mmc_send_tuning(). I was just looking at mtk-sd, which sometimes seems to care about whether the memcmp fails, while in others it doesn't. Honestly, I don't recall why so. Now, if we add a host cap, along the lines of what you propose, mtk-sd may not be able to use it, as it means the memcmp will always be skipped. > > The whole point is if the controller has the HW capability to check > the tuning block, so data->error in mmc_send_tuning will let mmc core > know if it need to manually compare the raw tuning block data. Right, so to be clear, this works for me as well! I just wanted to raise the idea above, with using a specific error code. However, may I suggest "MMC_CAP2_HW_TUNING" instead of MMC_CAP2_NO_TUNING_CMP, as to try to better describe the behavior of the HW? Also, if I am going to pick a patch adding this new cap, I would also like to get at least one mmc host that use it, so please add a patch on top for that. [...] Kind regards Uffe
On 2019/3/6 16:48, Ulf Hansson wrote: > On Wed, 6 Mar 2019 at 01:45, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> >> Hi Ulf >> >> On 2019/3/5 17:31, Ulf Hansson wrote: >>> On Fri, 1 Mar 2019 at 08:58, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>>> >>>> This caps2 is to skip doing memory compare tuning block >>>> pattern with the received data from devices when doing >>>> tuning. >>>> >>>> The data from device is all formatted like: >>>> Start bit + data + CRC + End bit >>>> >>>> For normal data read, we just verify host sample the data >>>> w/o any errors, but not need care the data context. But for >>>> a tuning process, we now compare the data context with JEDEC >>>> pattern. I highly suspect the rational behide it for the following >>>> details: >>>> >>>> (1) The tuning process is mostly used for identifing the rational >>>> sample window, so if the host read the tuning block successfully >>>> like a noraml data read, isn't it enough for sure that *this* single >>>> sample phase is good to work, no? The original patch to support it >>>> didn't quote any spec details but let's see the JESD84-B51, section 6.6.5, >>>> "Bus Sampling Tuning Concept" just says "This example is given for >>>> concept explanation purpose. The Host may use any other implementation >>>> according to its design consideration. " The concept isn't the order, >>>> and whether the MMC core need to verify the tuning block? See my argue (2). >>>> >>>> (2) AFAICT, the tuning block is handled by a fixed HW module within >>>> the device, if a JEDEC compatible device receive tuning block cmd and >>>> response it, could it sends back a non-compatible tuning block? >>> >>> It's hard to tell whether the above makes sense. I think only the HW >>> designers can give us a good answer on this, have you talked with >>> someone that can confirm your analyze? >>> >> >> I haven't talked with HW designers from device vendors. > > You don't need to card vendors, but your own HW guys that develops > your mmc controller. But, I assume you have done that, just wanted to > confirm it!? yes, I did. > > The point is, that you are referring to the JEDEC spec in the > changelog, which in the end doesn't described the internal specific > behavior of your mmc controller. Perhaps you could clarify that in the > next version of the changelog. > >> >>>> >>>> (3) Some(it's impossible for me to check all) of the SDHCI IP instances >>>> at hand which do tuning by HW block,never check the tuning block at all. >>> >>> I don't think that's correct, at least by looking at the result from: >>> >>> git grep -C 5 mmc_send_tuning >>> >>> To me it looks like it's only mtk-sd, that in some case only checks >>> the cmd errors, but in others the error check also includes the memcmp >>> with the tuning block. >>> >>> In other words, what you propose is something entirely new. >>> >>>> >>>> I'm intended to remove it entirely but just stay back a bit for >>>> skipping that if any vendors are willing to try(At least rockchip does). >>> >>> An option to the host cap, could be to return a special error code, >>> like -EPROTO instead of -EIO as currently, in case the memcmp with the >>> tuning block fails. >>> >>> This would allow the host to dynamically decide when it makes sense to >>> check for the error code, and when it doesn't. >>> >>> Would that work for you? >> >> I don't get your point. What I mean is to skip doing memcmp but not >> checking whether or why memcmp failed after calling mmc_send_tuning(). > > I was just looking at mtk-sd, which sometimes seems to care about > whether the memcmp fails, while in others it doesn't. Honestly, I > don't recall why so. > > Now, if we add a host cap, along the lines of what you propose, mtk-sd > may not be able to use it, as it means the memcmp will always be > skipped. > >> >> The whole point is if the controller has the HW capability to check >> the tuning block, so data->error in mmc_send_tuning will let mmc core >> know if it need to manually compare the raw tuning block data. > > Right, so to be clear, this works for me as well! I just wanted to > raise the idea above, with using a specific error code. > > However, may I suggest "MMC_CAP2_HW_TUNING" instead of > MMC_CAP2_NO_TUNING_CMP, as to try to better describe the behavior of > the HW? > > Also, if I am going to pick a patch adding this new cap, I would also > like to get at least one mmc host that use it, so please add a patch > on top for that. Ok, I got it. > > [...] > > Kind regards > Uffe > > >
> > On 2019/3/6 16:48, Ulf Hansson wrote: > > On Wed, 6 Mar 2019 at 01:45, Shawn Lin <shawn.lin@rock-chips.com> > wrote: > >> > >> Hi Ulf > >> > >> On 2019/3/5 17:31, Ulf Hansson wrote: > >>> On Fri, 1 Mar 2019 at 08:58, Shawn Lin <shawn.lin@rock-chips.com> > wrote: > >>>> > >>>> This caps2 is to skip doing memory compare tuning block > >>>> pattern with the received data from devices when doing > >>>> tuning. > >>>> > >>>> The data from device is all formatted like: > >>>> Start bit + data + CRC + End bit > >>>> > >>>> For normal data read, we just verify host sample the data > >>>> w/o any errors, but not need care the data context. But for > >>>> a tuning process, we now compare the data context with JEDEC > >>>> pattern. I highly suspect the rational behide it for the following > >>>> details: > >>>> > >>>> (1) The tuning process is mostly used for identifing the rational > >>>> sample window, so if the host read the tuning block successfully > >>>> like a noraml data read, isn't it enough for sure that *this* single > >>>> sample phase is good to work, no? The original patch to support it > >>>> didn't quote any spec details but let's see the JESD84-B51, section > 6.6.5, > >>>> "Bus Sampling Tuning Concept" just says "This example is given for > >>>> concept explanation purpose. The Host may use any other > implementation > >>>> according to its design consideration. " The concept isn't the order, > >>>> and whether the MMC core need to verify the tuning block? See my > argue (2). > >>>> > >>>> (2) AFAICT, the tuning block is handled by a fixed HW module within > >>>> the device, if a JEDEC compatible device receive tuning block cmd and > >>>> response it, could it sends back a non-compatible tuning block? > >>> > >>> It's hard to tell whether the above makes sense. I think only the HW > >>> designers can give us a good answer on this, have you talked with > >>> someone that can confirm your analyze? > >>> > >> > >> I haven't talked with HW designers from device vendors. > > > > You don't need to card vendors, but your own HW guys that develops > > your mmc controller. But, I assume you have done that, just wanted to > > confirm it!? > > yes, I did. I can also consult our device hw designers, maybe they can provide some further insight. Please allow a day or two for that. Thanks, Avri
> >>>> For normal data read, we just verify host sample the data > >>>> w/o any errors, but not need care the data context. But for > >>>> a tuning process, we now compare the data context with JEDEC > >>>> pattern. I highly suspect the rational behide it for the following Behide = behind > >>>> details: > >>>> > >>>> (1) The tuning process is mostly used for identifing the rational > >>>> sample window, so if the host read the tuning block successfully > >>>> like a noraml data read, isn't it enough for sure that *this* single > >>>> sample phase is good to work, no? The original patch to support it > >>>> didn't quote any spec details but let's see the JESD84-B51, section > 6.6.5, > >>>> "Bus Sampling Tuning Concept" just says "This example is given for > >>>> concept explanation purpose. The Host may use any other > implementation > >>>> according to its design consideration. " The concept isn't the order, > >>>> and whether the MMC core need to verify the tuning block? See my > argue (2). Maybe you could reword paragraph (1) - It is hard to follow your line of reasoning > >>>> > >>>> (2) AFAICT, the tuning block is handled by a fixed HW module within > >>>> the device, if a JEDEC compatible device receive tuning block cmd and > >>>> response it, could it sends back a non-compatible tuning block? > >>> > >>> It's hard to tell whether the above makes sense. I think only the HW > >>> designers can give us a good answer on this, have you talked with > >>> someone that can confirm your analyze? > >>> > >> > >> I haven't talked with HW designers from device vendors. Ack on that. Thanks, Avri
On 2019/3/11 4:25, Avri Altman wrote: >>>>>> For normal data read, we just verify host sample the data >>>>>> w/o any errors, but not need care the data context. But for >>>>>> a tuning process, we now compare the data context with JEDEC >>>>>> pattern. I highly suspect the rational behide it for the following > Behide = behind > >>>>>> details: >>>>>> >>>>>> (1) The tuning process is mostly used for identifing the rational >>>>>> sample window, so if the host read the tuning block successfully >>>>>> like a noraml data read, isn't it enough for sure that *this* single >>>>>> sample phase is good to work, no? The original patch to support it >>>>>> didn't quote any spec details but let's see the JESD84-B51, section >> 6.6.5, >>>>>> "Bus Sampling Tuning Concept" just says "This example is given for >>>>>> concept explanation purpose. The Host may use any other >> implementation >>>>>> according to its design consideration. " The concept isn't the order, >>>>>> and whether the MMC core need to verify the tuning block? See my >> argue (2). > Maybe you could reword paragraph (1) - > It is hard to follow your line of reasoning Sure, will post a normal patch if got ready and reword it. > >>>>>> >>>>>> (2) AFAICT, the tuning block is handled by a fixed HW module within >>>>>> the device, if a JEDEC compatible device receive tuning block cmd and >>>>>> response it, could it sends back a non-compatible tuning block? >>>>> >>>>> It's hard to tell whether the above makes sense. I think only the HW >>>>> designers can give us a good answer on this, have you talked with >>>>> someone that can confirm your analyze? >>>>> >>>> >>>> I haven't talked with HW designers from device vendors. > Ack on that. Thanks. > > Thanks, > Avri >
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c index c5208fb..802cba5 100644 --- a/drivers/mmc/core/mmc_ops.c +++ b/drivers/mmc/core/mmc_ops.c @@ -664,7 +664,8 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error) goto out; } - if (memcmp(data_buf, tuning_block_pattern, size)) + if (!(host->caps & MMC_CAP2_NO_TUNING_CMP) && + memcmp(data_buf, tuning_block_pattern, size)) err = -EIO; out: diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 43d0f0c..8e759ff 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -370,6 +370,7 @@ struct mmc_host { #define MMC_CAP2_CQE (1 << 23) /* Has eMMC command queue engine */ #define MMC_CAP2_CQE_DCMD (1 << 24) /* CQE can issue a direct command */ #define MMC_CAP2_AVOID_3_3V (1 << 25) /* Host must negotiate down from 3.3V */ +#define MMC_CAP2_NO_TUNING_CMP (1 << 25) /* Do not need compare the tuning block */ int fixed_drv_type; /* fixed driver type for non-removable media */
This caps2 is to skip doing memory compare tuning block pattern with the received data from devices when doing tuning. The data from device is all formatted like: Start bit + data + CRC + End bit For normal data read, we just verify host sample the data w/o any errors, but not need care the data context. But for a tuning process, we now compare the data context with JEDEC pattern. I highly suspect the rational behide it for the following details: (1) The tuning process is mostly used for identifing the rational sample window, so if the host read the tuning block successfully like a noraml data read, isn't it enough for sure that *this* single sample phase is good to work, no? The original patch to support it didn't quote any spec details but let's see the JESD84-B51, section 6.6.5, "Bus Sampling Tuning Concept" just says "This example is given for concept explanation purpose. The Host may use any other implementation according to its design consideration. " The concept isn't the order, and whether the MMC core need to verify the tuning block? See my argue (2). (2) AFAICT, the tuning block is handled by a fixed HW module within the device, if a JEDEC compatible device receive tuning block cmd and response it, could it sends back a non-compatible tuning block? (3) Some(it's impossible for me to check all) of the SDHCI IP instances at hand which do tuning by HW block,never check the tuning block at all. I'm intended to remove it entirely but just stay back a bit for skipping that if any vendors are willing to try(At least rockchip does). Any comments will be appreciated. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/core/mmc_ops.c | 3 ++- include/linux/mmc/host.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)