| Message ID | 1462527673-24711-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Shawn, On Fri, May 6, 2016 at 2:41 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > rockchip,default-drv-phase is used to set the default drv degrees. > drv phases will decide the write timing of mmc controller. Device tree bindings should probably have been patch 1/2, then the code patch 2/2. > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt > index ea5614b..c48dba6 100644 > --- a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt > +++ b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt > @@ -29,6 +29,9 @@ Optional Properties: > probing, low speeds or in case where all phases work at tuning time. > If not specified 0 deg will be used. > > +* rockchip,default-drv-phase: The default phase to set ciu_drv at probing > + for host to write data to devices. If not specified 180 deg will be used. This is probably not right for a few reasons. 1. Specifying a single number for this property in terms of "degrees" is probably not right. The whole point of setting the "drive phase" is to meet hold times, which are specified in the spec in terms of ns in the spec and also specified differently for different SD/MMC speed modes. Note also that "phase" translates to very different delays (in terms of ns) depending on the clock rate: At 400 kHz, period is 2.5 us, so 90 degree phase offset is a delay of 625 ns At 25 MHz, period is 40 ns, so a 90 degree phase offset represents a delay of 10 ns. At 50 MHz, period is 20 ns, so a 90 degree phase offset represents a delay of 5 ns. At 200 MHz, period is period is 5 ns, so a 90 degree phase offset represents a delay of 1.25 ns. 2. As I understand it, the value needed for the drive phase is not board specific unless you've got super crazy layout on a board (where the clock line takes a very different path than everything else). It's also not even terribly SoC-specific unless you've got some very strange incarnation of dw_mmc that has very different internal delays than everyone else. Said another way, until we see an instance of an SoC/board that really needs to do things special I'd say that we should just implement this all in code (no device tree bindings). 3. If this property was actually board specific and actually needed to be tuned board-by-board, you'd have a bug because your new device tree bindings are not backward compatible and you'd probably be breaking old boards. Specifically you're changing the definition of what happens when "rockchip,default-drv-phase" is not specified. Old behavior was to leave the value that was setup by the firmware (or perhaps the hardware default if the firmware didn't touch this). --- OK, so what should we do? We could certainly do lots of crazy math to come up with the ideal hold time for all different speed modes and all different types of cards. With my reading of the Designware Databook this would mean that somewhere we'd want to specify which delay method we're using (phase shift vs. delay line) and how long all the delays timings all are on your particular SoC. That all sounds quite difficult, though. Probably you could just add a simple function that looked at the clock and speed mode and always chose an offset of 90 or 180 degrees. At least on Rockchip devices you can be certain that you can make 90 and 180 degrees using phase shifts and thus the timings should be consistent. By default you could just always choose 180. The Designware databook has some examples where it picked 90 degrees (SDR50, DDR50, SDR25, MMC High Speed), but I'm not enough of an MMC expert to know if there is some benefit to choosing 90. Would we violate any specs if we just chose 180 degrees all the time everywhere on all Rockchip devices? -Doug
On 2016/5/7 1:26, Doug Anderson wrote: > Shawn, > > On Fri, May 6, 2016 at 2:41 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> rockchip,default-drv-phase is used to set the default drv degrees. >> drv phases will decide the write timing of mmc controller. > > Device tree bindings should probably have been patch 1/2, then the > code patch 2/2. > > >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt >> index ea5614b..c48dba6 100644 >> --- a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt >> +++ b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt >> @@ -29,6 +29,9 @@ Optional Properties: >> probing, low speeds or in case where all phases work at tuning time. >> If not specified 0 deg will be used. >> >> +* rockchip,default-drv-phase: The default phase to set ciu_drv at probing >> + for host to write data to devices. If not specified 180 deg will be used. > > This is probably not right for a few reasons. > > 1. Specifying a single number for this property in terms of "degrees" > is probably not right. The whole point of setting the "drive phase" > is to meet hold times, which are specified in the spec in terms of ns > in the spec and also specified differently for different SD/MMC speed > modes. Note also that "phase" translates to very different delays (in > terms of ns) depending on the clock rate: > > At 400 kHz, period is 2.5 us, so 90 degree phase offset is a delay of 625 ns > At 25 MHz, period is 40 ns, so a 90 degree phase offset represents a > delay of 10 ns. > At 50 MHz, period is 20 ns, so a 90 degree phase offset represents a > delay of 5 ns. > At 200 MHz, period is period is 5 ns, so a 90 degree phase offset > represents a delay of 1.25 ns. yes, if we use degrees only(0/90/180/270), the timing is always right. But considering the delay number, we need to do some crazy calculation in the set_ios callback. > > > 2. As I understand it, the value needed for the drive phase is not > board specific unless you've got super crazy layout on a board (where > the clock line takes a very different path than everything else). > It's also not even terribly SoC-specific unless you've got some very > strange incarnation of dw_mmc that has very different internal delays > than everyone else. Said another way, until we see an instance of an > SoC/board that really needs to do things special I'd say that we > should just implement this all in code (no device tree bindings). > I'm prone to think it should be Soc specific if making sure the layout for data lines is in equal length. > > > 3. If this property was actually board specific and actually needed to > be tuned board-by-board, you'd have a bug because your new device tree > bindings are not backward compatible and you'd probably be breaking > old boards. Specifically you're changing the definition of what > happens when "rockchip,default-drv-phase" is not specified. Old > behavior was to leave the value that was setup by the firmware (or > perhaps the hardware default if the firmware didn't touch this). drv_phase is for all the data lines instead of tuning the lines one-by-one. So this patch can't save the terrible board layout. But I agree that it will break the compatibility backward if firware touch this value. > > --- > > OK, so what should we do? > > We could certainly do lots of crazy math to come up with the ideal > hold time for all different speed modes and all different types of > cards. With my reading of the Designware Databook this would mean > that somewhere we'd want to specify which delay method we're using > (phase shift vs. delay line) and how long all the delays timings all > are on your particular SoC. That all sounds quite difficult, though. delay line is diff from chip to chip, soc-to-soc, board-to-board. For sample-phase we have tuning process and re-tune, but not for drv-phase. So We bascially should avoid to use it for drv-phase. Another consideration is the temperature drift of delay line. Maybe we should do some tricky limitation on clk-mmc-phase to only support fixed degrees? > > Probably you could just add a simple function that looked at the clock > and speed mode and always chose an offset of 90 or 180 degrees. At > least on Rockchip devices you can be certain that you can make 90 and > 180 degrees using phase shifts and thus the timings should be > consistent. By default you could just always choose 180. The > Designware databook has some examples where it picked 90 degrees > (SDR50, DDR50, SDR25, MMC High Speed), but I'm not enough of an MMC > expert to know if there is some benefit to choosing 90. Would we > violate any specs if we just chose 180 degrees all the time everywhere > on all Rockchip devices? It needs more waveform test to see how things going. But most of rockchip platforms in the pass years didn't touch drv-phase stuff not only in kernel but also in firmware, then we still cannot see the violation against the spec when using defalut reset value, namely 180, for drv-phase. > > > > -Doug > > >
Hi, On Mon, May 9, 2016 at 4:12 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> 1. Specifying a single number for this property in terms of "degrees" >> is probably not right. The whole point of setting the "drive phase" >> is to meet hold times, which are specified in the spec in terms of ns >> in the spec and also specified differently for different SD/MMC speed >> modes. Note also that "phase" translates to very different delays (in >> terms of ns) depending on the clock rate: >> >> At 400 kHz, period is 2.5 us, so 90 degree phase offset is a delay of 625 >> ns >> At 25 MHz, period is 40 ns, so a 90 degree phase offset represents a >> delay of 10 ns. >> At 50 MHz, period is 20 ns, so a 90 degree phase offset represents a >> delay of 5 ns. >> At 200 MHz, period is period is 5 ns, so a 90 degree phase offset >> represents a delay of 1.25 ns. > > > yes, if we use degrees only(0/90/180/270), the timing is always right. > But considering the delay number, we need to do some crazy calculation > in the set_ios callback. Great, so let's limit it to 0/90/180/270 for the drive phase offset. We don't need lots of precision for the drive phase offset (right?) and accuracy is more important. >> 2. As I understand it, the value needed for the drive phase is not >> board specific unless you've got super crazy layout on a board (where >> the clock line takes a very different path than everything else). >> It's also not even terribly SoC-specific unless you've got some very >> strange incarnation of dw_mmc that has very different internal delays >> than everyone else. Said another way, until we see an instance of an >> SoC/board that really needs to do things special I'd say that we >> should just implement this all in code (no device tree bindings). >> > > I'm prone to think it should be Soc specific if making sure the layout > for data lines is in equal length. Sure, it can be SoC specific. ...though at the moment, I'd bet that you can come up with a single rule for the drive phase offset that will work for every Rockchip SoC produced so far, especially if you are using only 0/90/180/270. I'd imagine that they all have similar enough internal delays. >> 3. If this property was actually board specific and actually needed to >> be tuned board-by-board, you'd have a bug because your new device tree >> bindings are not backward compatible and you'd probably be breaking >> old boards. Specifically you're changing the definition of what >> happens when "rockchip,default-drv-phase" is not specified. Old >> behavior was to leave the value that was setup by the firmware (or >> perhaps the hardware default if the firmware didn't touch this). > > > drv_phase is for all the data lines instead of tuning the lines > one-by-one. So this patch can't save the terrible board layout. > But I agree that it will break the compatibility backward if firware > touch this value. > >> >> --- >> >> OK, so what should we do? >> >> We could certainly do lots of crazy math to come up with the ideal >> hold time for all different speed modes and all different types of >> cards. With my reading of the Designware Databook this would mean >> that somewhere we'd want to specify which delay method we're using >> (phase shift vs. delay line) and how long all the delays timings all >> are on your particular SoC. That all sounds quite difficult, though. > > > delay line is diff from chip to chip, soc-to-soc, board-to-board. For > sample-phase we have tuning process and re-tune, but not for drv-phase. > So We bascially should avoid to use it for drv-phase. Another > consideration is the temperature drift of delay line. > > Maybe we should do some tricky limitation on clk-mmc-phase to only > support fixed degrees? As per above, let's not use delay line for drive delay. On all Rockchip SoCs that I have seen it's possible to make 0/90/180/270 very accurately. That means no board-to-board differences. Current mainline Linux kernel source code will always make 0/90/180/270 using phase offset (not delay line). For sampling we use tuning and using the delay elements makes some sense (since 90 degrees is not quite accurate enough to fully tune UHS). ...but for driving where the only requirement is hold times we don't need delay elements. >> Probably you could just add a simple function that looked at the clock >> and speed mode and always chose an offset of 90 or 180 degrees. At >> least on Rockchip devices you can be certain that you can make 90 and >> 180 degrees using phase shifts and thus the timings should be >> consistent. By default you could just always choose 180. The >> Designware databook has some examples where it picked 90 degrees >> (SDR50, DDR50, SDR25, MMC High Speed), but I'm not enough of an MMC >> expert to know if there is some benefit to choosing 90. Would we >> violate any specs if we just chose 180 degrees all the time everywhere >> on all Rockchip devices? > > > It needs more waveform test to see how things going. But most of > rockchip platforms in the pass years didn't touch drv-phase stuff not > only in kernel but also in firmware, then we still cannot see the > violation against the spec when using defalut reset value, namely 180, for > drv-phase. Right, most Rockchip platforms simply don't touch this and it works OK. ...but I don't think it defaults to 180. Grepping through on my veyron (rk3288) device shows sdio1_drv - 90 sdio0_drv - 90 sdmmc_drv - 90 emmc_drv -180 ...and, as we've seen, these values appear to be 270 on some other SoCs. The claim from the Designware Databook says that SDR104 and SDR12, and identification mode need 180 degrees to work properly. It would be interesting to hook up rk3288 to a signal analyzer and see hold times are OK or if we need to move up to 180 degrees for those modes. Note that the Designware Databook assumes "Delay_O" of 1.4ns. If yours is shorter then maybe 90 degrees is OK for those modes? Also: I still haven't heard whether there is any downside to using 180 degrees for modes that only require 90 degrees. Does it cause problems to just always use 180 degrees? If not, we could possibly use 180 degrees everywhere and just hardcode it? ...but if 180 is ideal everywhere, can you answer: * Why does dw_mmc manual spend so much time talking about it? It could just say "set to 180 degrees". * Why do Rockchip SoCs default to values that are not 180 degrees? -Doug
? 2016/5/10 0:31, Doug Anderson ??: > Hi, > > On Mon, May 9, 2016 at 4:12 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> 1. Specifying a single number for this property in terms of "degrees" >>> is probably not right. The whole point of setting the "drive phase" >>> is to meet hold times, which are specified in the spec in terms of ns >>> in the spec and also specified differently for different SD/MMC speed >>> modes. Note also that "phase" translates to very different delays (in >>> terms of ns) depending on the clock rate: >>> >>> At 400 kHz, period is 2.5 us, so 90 degree phase offset is a delay of 625 >>> ns >>> At 25 MHz, period is 40 ns, so a 90 degree phase offset represents a >>> delay of 10 ns. >>> At 50 MHz, period is 20 ns, so a 90 degree phase offset represents a >>> delay of 5 ns. >>> At 200 MHz, period is period is 5 ns, so a 90 degree phase offset >>> represents a delay of 1.25 ns. >> >> >> yes, if we use degrees only(0/90/180/270), the timing is always right. >> But considering the delay number, we need to do some crazy calculation >> in the set_ios callback. > > Great, so let's limit it to 0/90/180/270 for the drive phase offset. > We don't need lots of precision for the drive phase offset (right?) > and accuracy is more important. yes. > > >>> 2. As I understand it, the value needed for the drive phase is not >>> board specific unless you've got super crazy layout on a board (where >>> the clock line takes a very different path than everything else). >>> It's also not even terribly SoC-specific unless you've got some very >>> strange incarnation of dw_mmc that has very different internal delays >>> than everyone else. Said another way, until we see an instance of an >>> SoC/board that really needs to do things special I'd say that we >>> should just implement this all in code (no device tree bindings). >>> >> >> I'm prone to think it should be Soc specific if making sure the layout >> for data lines is in equal length. > > Sure, it can be SoC specific. ...though at the moment, I'd bet that > you can come up with a single rule for the drive phase offset that > will work for every Rockchip SoC produced so far, especially if you > are using only 0/90/180/270. I'd imagine that they all have similar > enough internal delays. For a specific Soc, it's the basic rule to make sure the internal delays is the same(nearly the same) for all of the lines. > > >>> 3. If this property was actually board specific and actually needed to >>> be tuned board-by-board, you'd have a bug because your new device tree >>> bindings are not backward compatible and you'd probably be breaking >>> old boards. Specifically you're changing the definition of what >>> happens when "rockchip,default-drv-phase" is not specified. Old >>> behavior was to leave the value that was setup by the firmware (or >>> perhaps the hardware default if the firmware didn't touch this). >> >> >> drv_phase is for all the data lines instead of tuning the lines >> one-by-one. So this patch can't save the terrible board layout. >> But I agree that it will break the compatibility backward if firware >> touch this value. >> >>> >>> --- >>> >>> OK, so what should we do? >>> >>> We could certainly do lots of crazy math to come up with the ideal >>> hold time for all different speed modes and all different types of >>> cards. With my reading of the Designware Databook this would mean >>> that somewhere we'd want to specify which delay method we're using >>> (phase shift vs. delay line) and how long all the delays timings all >>> are on your particular SoC. That all sounds quite difficult, though. >> >> >> delay line is diff from chip to chip, soc-to-soc, board-to-board. For >> sample-phase we have tuning process and re-tune, but not for drv-phase. >> So We bascially should avoid to use it for drv-phase. Another >> consideration is the temperature drift of delay line. >> >> Maybe we should do some tricky limitation on clk-mmc-phase to only >> support fixed degrees? > > As per above, let's not use delay line for drive delay. On all > Rockchip SoCs that I have seen it's possible to make 0/90/180/270 very > accurately. That means no board-to-board differences. Current > mainline Linux kernel source code will always make 0/90/180/270 using > phase offset (not delay line). yes, 0/90/180/270 is very accurate and mainline kernel use phase offset for these four phase. > > For sampling we use tuning and using the delay elements makes some > sense (since 90 degrees is not quite accurate enough to fully tune > UHS). ...but for driving where the only requirement is hold times we > don't need delay elements. Right. We care hold time here. > > >>> Probably you could just add a simple function that looked at the clock >>> and speed mode and always chose an offset of 90 or 180 degrees. At >>> least on Rockchip devices you can be certain that you can make 90 and >>> 180 degrees using phase shifts and thus the timings should be >>> consistent. By default you could just always choose 180. The >>> Designware databook has some examples where it picked 90 degrees >>> (SDR50, DDR50, SDR25, MMC High Speed), but I'm not enough of an MMC >>> expert to know if there is some benefit to choosing 90. Would we >>> violate any specs if we just chose 180 degrees all the time everywhere >>> on all Rockchip devices? >> >> >> It needs more waveform test to see how things going. But most of >> rockchip platforms in the pass years didn't touch drv-phase stuff not >> only in kernel but also in firmware, then we still cannot see the >> violation against the spec when using defalut reset value, namely 180, for >> drv-phase. > > Right, most Rockchip platforms simply don't touch this and it works > OK. ...but I don't think it defaults to 180. Grepping through on my > veyron (rk3288) device shows > > sdio1_drv - 90 > sdio0_drv - 90 > sdmmc_drv - 90 > emmc_drv -180 > > ...and, as we've seen, these values appear to be 270 on some other SoCs. > Have your code touched them? I check the TRM and find it should be 180 always. Also for rk3036/3368/3399/3228.... the reset vaule is 180... > > The claim from the Designware Databook says that SDR104 and SDR12, and > identification mode need 180 degrees to work properly. It would be > interesting to hook up rk3288 to a signal analyzer and see hold times > are OK or if we need to move up to 180 degrees for those modes. > > Note that the Designware Databook assumes "Delay_O" of 1.4ns. If > yours is shorter then maybe 90 degrees is OK for those modes? > maybe. But I think 180(downside) is the better. > > Also: I still haven't heard whether there is any downside to using 180 > degrees for modes that only require 90 degrees. Does it cause > problems to just always use 180 degrees? If not, we could possibly > use 180 degrees everywhere and just hardcode it? From tons of test on rockchip products which always use 180, I didn't see failure. Hardcoding it doesn't look so decent maybe... but anyway it works. > > > ...but if 180 is ideal everywhere, can you answer: > > * Why does dw_mmc manual spend so much time talking about it? It > could just say "set to 180 degrees". I'm sure all the rockchip Socs defaultly use 180 for drv_phase if I got the right TRMs these years. Anyway let me check it with my ASIC team and I will let you know the result. Thanks. > > * Why do Rockchip SoCs default to values that are not 180 degrees? > > -Doug > > >
(again, but not HTML) On Tue, May 10, 2016 at 3:19 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > ? 2016/5/10 0:31, Doug Anderson ??: >> >> Hi, >> >> On Mon, May 9, 2016 at 4:12 AM, Shawn Lin <shawn.lin@rock-chips.com> >> wrote: >>>> >>>> 1. Specifying a single number for this property in terms of "degrees" >>>> is probably not right. The whole point of setting the "drive phase" >>>> is to meet hold times, which are specified in the spec in terms of ns >>>> in the spec and also specified differently for different SD/MMC speed >>>> modes. Note also that "phase" translates to very different delays (in >>>> terms of ns) depending on the clock rate: >>>> >>>> At 400 kHz, period is 2.5 us, so 90 degree phase offset is a delay of >>>> 625 >>>> ns >>>> At 25 MHz, period is 40 ns, so a 90 degree phase offset represents a >>>> delay of 10 ns. >>>> At 50 MHz, period is 20 ns, so a 90 degree phase offset represents a >>>> delay of 5 ns. >>>> At 200 MHz, period is period is 5 ns, so a 90 degree phase offset >>>> represents a delay of 1.25 ns. >>> >>> >>> >>> yes, if we use degrees only(0/90/180/270), the timing is always right. >>> But considering the delay number, we need to do some crazy calculation >>> in the set_ios callback. >> >> >> Great, so let's limit it to 0/90/180/270 for the drive phase offset. >> We don't need lots of precision for the drive phase offset (right?) >> and accuracy is more important. > > > yes. > >> >> >>>> 2. As I understand it, the value needed for the drive phase is not >>>> board specific unless you've got super crazy layout on a board (where >>>> the clock line takes a very different path than everything else). >>>> It's also not even terribly SoC-specific unless you've got some very >>>> strange incarnation of dw_mmc that has very different internal delays >>>> than everyone else. Said another way, until we see an instance of an >>>> SoC/board that really needs to do things special I'd say that we >>>> should just implement this all in code (no device tree bindings). >>>> >>> >>> I'm prone to think it should be Soc specific if making sure the layout >>> for data lines is in equal length. >> >> >> Sure, it can be SoC specific. ...though at the moment, I'd bet that >> you can come up with a single rule for the drive phase offset that >> will work for every Rockchip SoC produced so far, especially if you >> are using only 0/90/180/270. I'd imagine that they all have similar >> enough internal delays. > > > For a specific Soc, it's the basic rule to make sure the internal delays > is the same(nearly the same) for all of the lines. Right. I was saying that not only are all lines within a given SoC similar in terms of delay, but I was suggesting that perhaps the internal delay for rk3188, rk3288, rk3228, ... is probably fairly similar. That's only a guess, though. >>>> 3. If this property was actually board specific and actually needed to >>>> be tuned board-by-board, you'd have a bug because your new device tree >>>> bindings are not backward compatible and you'd probably be breaking >>>> old boards. Specifically you're changing the definition of what >>>> happens when "rockchip,default-drv-phase" is not specified. Old >>>> behavior was to leave the value that was setup by the firmware (or >>>> perhaps the hardware default if the firmware didn't touch this). >>> >>> >>> >>> drv_phase is for all the data lines instead of tuning the lines >>> one-by-one. So this patch can't save the terrible board layout. >>> But I agree that it will break the compatibility backward if firware >>> touch this value. >>> >>>> >>>> --- >>>> >>>> OK, so what should we do? >>>> >>>> We could certainly do lots of crazy math to come up with the ideal >>>> hold time for all different speed modes and all different types of >>>> cards. With my reading of the Designware Databook this would mean >>>> that somewhere we'd want to specify which delay method we're using >>>> (phase shift vs. delay line) and how long all the delays timings all >>>> are on your particular SoC. That all sounds quite difficult, though. >>> >>> >>> >>> delay line is diff from chip to chip, soc-to-soc, board-to-board. For >>> sample-phase we have tuning process and re-tune, but not for drv-phase. >>> So We bascially should avoid to use it for drv-phase. Another >>> consideration is the temperature drift of delay line. >>> >>> Maybe we should do some tricky limitation on clk-mmc-phase to only >>> support fixed degrees? >> >> >> As per above, let's not use delay line for drive delay. On all >> Rockchip SoCs that I have seen it's possible to make 0/90/180/270 very >> accurately. That means no board-to-board differences. Current >> mainline Linux kernel source code will always make 0/90/180/270 using >> phase offset (not delay line). > > > yes, 0/90/180/270 is very accurate and mainline kernel use phase offset > for these four phase. > >> >> For sampling we use tuning and using the delay elements makes some >> sense (since 90 degrees is not quite accurate enough to fully tune >> UHS). ...but for driving where the only requirement is hold times we >> don't need delay elements. > > > Right. We care hold time here. > >> >> >>>> Probably you could just add a simple function that looked at the clock >>>> and speed mode and always chose an offset of 90 or 180 degrees. At >>>> least on Rockchip devices you can be certain that you can make 90 and >>>> 180 degrees using phase shifts and thus the timings should be >>>> consistent. By default you could just always choose 180. The >>>> Designware databook has some examples where it picked 90 degrees >>>> (SDR50, DDR50, SDR25, MMC High Speed), but I'm not enough of an MMC >>>> expert to know if there is some benefit to choosing 90. Would we >>>> violate any specs if we just chose 180 degrees all the time everywhere >>>> on all Rockchip devices? >>> >>> >>> >>> It needs more waveform test to see how things going. But most of >>> rockchip platforms in the pass years didn't touch drv-phase stuff not >>> only in kernel but also in firmware, then we still cannot see the >>> violation against the spec when using defalut reset value, namely 180, >>> for >>> drv-phase. >> >> >> Right, most Rockchip platforms simply don't touch this and it works >> OK. ...but I don't think it defaults to 180. Grepping through on my >> veyron (rk3288) device shows >> >> sdio1_drv - 90 >> sdio0_drv - 90 >> sdmmc_drv - 90 >> emmc_drv -180 >> >> ...and, as we've seen, these values appear to be 270 on some other SoCs. >> > > Have your code touched them? I check the TRM and find it should be 180 > always. > > Also for rk3036/3368/3399/3228.... the reset vaule is 180... I have less faith than you in the TRM. The TRM is full of minor errors and is often wrong about the default state of things. IMHO the only true way to find out is to boot up some SoCs and check. As far as whether code touches these values: * I'm 100% certain that the kernel on rk3288 I tested doesn't touch. * I'm 100% certain that the kernel on rk3399 I tested doesn't touch. * I'm 95% certain that the BIOS (coreboot/depthcharge) on rk3288 I tested doesn't touch. * I'm 95% certain that the BIOS (coreboot/depthcharge) on rk3399 I tested doesn't touch. Note that the TRM on rk3399 does say 180 degrees is the default, but I believe you have also verified that it comes up as 270. Have you found any reason for this? Also note that I look at V1.1 of the rk3288 TRM in the CRU section and I see: * CRU_SDMMC_CON0: drive degree defaults to 0x1 * CRU_SDIO0_CON0: drive degree defaults to 0x1 * CRU_SDIO1_CON0: drive degree defaults to 0x1 * CRU_EMMC_CON0: drive degree defaults to 0x1 That actually means that they default to 90. Of course you can also see that the TRM is probably flawed because it claims that all the bits here are "WO" (write only). They are clearly not. The TRM is also internally inconsistent. When I then go into the Mobile Storage Host section (Variable Delay/Clock Generation), I see that it does in fact claim that the default is 180. >> The claim from the Designware Databook says that SDR104 and SDR12, and >> identification mode need 180 degrees to work properly. It would be >> interesting to hook up rk3288 to a signal analyzer and see hold times >> are OK or if we need to move up to 180 degrees for those modes. >> >> Note that the Designware Databook assumes "Delay_O" of 1.4ns. If >> yours is shorter then maybe 90 degrees is OK for those modes? >> > > maybe. But I think 180(downside) is the better. I'm OK with using 180 always as long as SD cards continue to work OK. Best would be if someone could actually run a protocol analyzer for all the different speed modes. >> Also: I still haven't heard whether there is any downside to using 180 >> degrees for modes that only require 90 degrees. Does it cause >> problems to just always use 180 degrees? If not, we could possibly >> use 180 degrees everywhere and just hardcode it? > > > From tons of test on rockchip products which always use 180, I didn't > see failure. Hardcoding it doesn't look so decent maybe... but anyway > it works. Hardcoding in this case is much better than putting in a device tree binding. I'm nearly certain that having a device tree binding that doesn't account for different clock rates / speed modes is not a good idea, and device tree bindings are supposed to be "set in stone". Let's just hardcode it for now. I can post up a CL. >> ...but if 180 is ideal everywhere, can you answer: >> >> * Why does dw_mmc manual spend so much time talking about it? It >> could just say "set to 180 degrees". > > > I'm sure all the rockchip Socs defaultly use 180 for drv_phase if I got > the right TRMs these years. > > Anyway let me check it with my ASIC team and I will let you know > the result. You probably have the right TRMs. The TRMs are just not accurate. ;)
On 2016/5/10 23:57, Doug Anderson wrote: > (again, but not HTML) > ------8<------------------ > > I have less faith than you in the TRM. The TRM is full of minor > errors and is often wrong about the default state of things. IMHO the > only true way to find out is to boot up some SoCs and check. > Okay, I should not have too much confidence on my TRM maybe :). Again, We SHOULD refer to the Mobile Storage Host section (Variable Delay/Clock Generation) instead of CRU section, otherwise even you will see inconsistent decription of mmc_clock->shift. > As far as whether code touches these values: > ---------8<----------------- >> >> maybe. But I think 180(downside) is the better. NAK my previous comments here. Downside is better for SRD, but won't work for DDR mode. When running in DDR mode, we should use 90 instead. So let me elaborate a bit more here. For DDR mode, one single clk cycle should sending two data bits outside to the devices. We need a hold time for both. If 180 is used, the first bit occurs around the downside area, which won't be sampled by devices on the upside. So on the upside, the devices will see a zero bit if you actually send a one-bit, which makes the devices generate CRC finally. For this above, 180 for all SDR mode is ok, but 90 should be deployed for DDR mode. So simply checking the timing to hardcode it should be fine. > > I'm OK with using 180 always as long as SD cards continue to work OK. > Best would be if someone could actually run a protocol analyzer for > all the different speed modes. > > >>> Also: I still haven't heard whether there is any downside to using 180 >>> degrees for modes that only require 90 degrees. Does it cause >>> problems to just always use 180 degrees? If not, we could possibly >>> use 180 degrees everywhere and just hardcode it? >> >>
Hi, On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> maybe. But I think 180(downside) is the better. > > > NAK my previous comments here. Downside is better for SRD, but won't > work for DDR mode. When running in DDR mode, we should use 90 instead. > > So let me elaborate a bit more here. > For DDR mode, one single clk cycle should sending two data bits outside > to the devices. We need a hold time for both. If 180 is used, the first > bit occurs around the downside area, which won't be sampled by devices > on the upside. So on the upside, the devices will see a zero bit if you > actually send a one-bit, which makes the devices generate CRC finally. > > > For this above, 180 for all SDR mode is ok, but 90 should be deployed > for DDR mode. So simply checking the timing to hardcode it should be > fine. OK, I sent out a patch for 180 always. I can send v2 to use 90 for DDR modes tomorrow. ...or feel free to post that yourself if you want. We want 90 for all DDR modes? So MMC_TIMING_UHS_DDR50, MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400 in dw_mmc on Rockchip). -Doug
On 2016/5/11 11:50, Doug Anderson wrote: > Hi, > > On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>>> maybe. But I think 180(downside) is the better. >> >> >> NAK my previous comments here. Downside is better for SRD, but won't >> work for DDR mode. When running in DDR mode, we should use 90 instead. >> >> So let me elaborate a bit more here. >> For DDR mode, one single clk cycle should sending two data bits outside >> to the devices. We need a hold time for both. If 180 is used, the first >> bit occurs around the downside area, which won't be sampled by devices >> on the upside. So on the upside, the devices will see a zero bit if you >> actually send a one-bit, which makes the devices generate CRC finally. >> >> >> For this above, 180 for all SDR mode is ok, but 90 should be deployed >> for DDR mode. So simply checking the timing to hardcode it should be >> fine. > > OK, I sent out a patch for 180 always. I can send v2 to use 90 for > DDR modes tomorrow. ...or feel free to post that yourself if you > want. > > We want 90 for all DDR modes? So MMC_TIMING_UHS_DDR50, > MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400 > in dw_mmc on Rockchip). Right. > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Shawn, On Wed, May 11, 2016 at 1:25 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2016/5/11 11:50, Doug Anderson wrote: >> >> Hi, >> >> On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com> >> wrote: >>>>> >>>>> maybe. But I think 180(downside) is the better. >>> >>> >>> >>> NAK my previous comments here. Downside is better for SRD, but won't >>> work for DDR mode. When running in DDR mode, we should use 90 instead. >>> >>> So let me elaborate a bit more here. >>> For DDR mode, one single clk cycle should sending two data bits outside >>> to the devices. We need a hold time for both. If 180 is used, the first >>> bit occurs around the downside area, which won't be sampled by devices >>> on the upside. So on the upside, the devices will see a zero bit if you >>> actually send a one-bit, which makes the devices generate CRC finally. >>> >>> >>> For this above, 180 for all SDR mode is ok, but 90 should be deployed >>> for DDR mode. So simply checking the timing to hardcode it should be >>> fine. >> >> >> OK, I sent out a patch for 180 always. I can send v2 to use 90 for >> DDR modes tomorrow. ...or feel free to post that yourself if you >> want. >> >> We want 90 for all DDR modes? So MMC_TIMING_UHS_DDR50, >> MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400 >> in dw_mmc on Rockchip). > > > Right. OK, so I dug into this more. I don't think it's actually quite that simple. The Designware databook explicitly states that 'MMC DDR 8-bit' should use 180, not 90. ...but that's probably explained by the fact that "cclk_in" for MMC-DDR 8-bit is double what you'd expect. You can see that in code: if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing == MMC_TIMING_MMC_DDR52) cclkin = 2 * ios->clock * RK3288_CLKGEN_DIV; --- tl;dr of all the below: * For SD: use 180 for SDR104 to get margin, use 90 elsewhere. * For mmc, use 180 for DDR52 to meet timings, use 180 for HS200 to get margin, use 90 elsewhere The nice thing about this is, at least for rk3288, things don't change much from today where we use 90 degrees for most SD cards. --- For new patch, see: <https://patchwork.kernel.org/patch/9075141/> --- Details: So assuming measurements I saw from an EE are correct, measured "delay_o_ns" is ~ .5 ns for rk3399. With that, we can do some math. Please correct any mistakes. Math is hard. Note that on all Rockchip devices I've seen pins are limited to 150 MHz and when we choose 150 MHz we end up at 148.5 MHz. Calculations aren't too different if we use 150 for that case. MMC_TIMING_MMC_DDR52: min hold time = 2.5 ns min data delay = 2.5 ns + .5 ns = 3 ns with 100 MHz clock, 90 degree: 2.5 ns with 100 MHz clock, 180 degree: 5.0 ns ...need 180 degree -- SD DDR50: min hold time = .8 ns min data delay = .8 ns + .5 ns = 1.3 ns with 50 MHz clock, 90 degree: 5 ns with 50 MHz clock, 180 degree: 10 ns ...90 degree is good and 180 is massive overkill. Use 90. -- SD SDR104 (limited to 148.5 MHz on existing Rockchip parts): min hold time = .8 ns min data delay = .8 ns + .5 ns = 1.3 ns with 148.5 MHz clock, 90 degree: 1.68 ns with 148.5 MHz clock, 180 degree: 3.37 ns ...so 90 degrees would probably work OK, but 180 degrees gives more margin. ...probably explains why existing rk3288 devices w/ 90 degree work OK. if we had 200 MHz clock, 90 degree: 1.25 ns if we had 200 MHz clock, 180 degree: 2.50 ns -- SD SDR50: min hold time = .8 ns min data delay = .8 ns + .5 ns = 1.3 ns with 100 MHz clock, 90 degree: 2.5 ns with 100 MHz clock, 180 degree: 5.0 ns ...90 degree is great w/ plenty of margin -- SD SDR25: min hold time = 2 ns min data delay = 2 ns + .5 ns = 2.5 ns with 50 MHz clock, 90 degree: 5 ns with 50 MHz clock, 180 degree: 10 ns ...90 degree is good and 180 is massive overkill. Use 90. -- SD SDR12 (databook shows cclk_in as 50 MHz here we'll get 25 MHz): min hold time = 5 ns min data delay = 5 ns + .5 ns = 5.5 ns with 25 MHz clock, 90 degree: 10 ns with 25 MHz clock, 180 degree: 20 ns ...90 degree is good (databook suggests 180 for this mode due to cclk_in = 50) -- ID Mode min hold time = 5 ns min data delay = 5 ns + .5 ns = 5.5 ns with 400 kHz clock, 90 degree: 625 ns with 400 kHz clock, 180 degree: 1250 ns ...90 degree is good (databook suggests 180 for this mode due to cclk_in = 50) -- MMC High speed: min hold time = 2.5 ns min data delay = 2.5 ns + .5 ns = 3 ns with 50 MHz clock, 90 degree: 5 ns with 50 MHz clock, 180 degree: 10 ns ...90 degree is good -- HS200: min hold time (tIH from JEDEC spec): .8 ns ...math all works out the same as SDR104. -- HS400: we don't support it anyway
? 2016/5/12 5:42, Doug Anderson ??: > Shawn, > > On Wed, May 11, 2016 at 1:25 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> On 2016/5/11 11:50, Doug Anderson wrote: >>> >>> Hi, >>> >>> On Tue, May 10, 2016 at 7:50 PM, Shawn Lin <shawn.lin@rock-chips.com> >>> wrote: >>>>>> >>>>>> maybe. But I think 180(downside) is the better. >>>> >>>> >>>> >>>> NAK my previous comments here. Downside is better for SRD, but won't >>>> work for DDR mode. When running in DDR mode, we should use 90 instead. >>>> >>>> So let me elaborate a bit more here. >>>> For DDR mode, one single clk cycle should sending two data bits outside >>>> to the devices. We need a hold time for both. If 180 is used, the first >>>> bit occurs around the downside area, which won't be sampled by devices >>>> on the upside. So on the upside, the devices will see a zero bit if you >>>> actually send a one-bit, which makes the devices generate CRC finally. >>>> >>>> >>>> For this above, 180 for all SDR mode is ok, but 90 should be deployed >>>> for DDR mode. So simply checking the timing to hardcode it should be >>>> fine. >>> >>> >>> OK, I sent out a patch for 180 always. I can send v2 to use 90 for >>> DDR modes tomorrow. ...or feel free to post that yourself if you >>> want. >>> >>> We want 90 for all DDR modes? So MMC_TIMING_UHS_DDR50, >>> MMC_TIMING_MMC_DDR52, MMC_TIMING_MMC_HS400? (not that we support HS400 >>> in dw_mmc on Rockchip). >> >> >> Right. > > OK, so I dug into this more. I don't think it's actually quite that > simple. The Designware databook explicitly states that 'MMC DDR > 8-bit' should use 180, not 90. ...but that's probably explained by > the fact that "cclk_in" for MMC-DDR 8-bit is double what you'd expect. > You can see that in code: > > if (ios->bus_width == MMC_BUS_WIDTH_8 && > ios->timing == MMC_TIMING_MMC_DDR52) > cclkin = 2 * ios->clock * RK3288_CLKGEN_DIV; Yes, this requirment is from dw_mmc databook(v270a), table 10-5. But you can see that we didn't handle MMC_TIMING_UHS_SDR12. As recommended, MMC_TIMING_UHS_SDR12 mode need to 50M here to make the internal devider to be "1". Because Figure 10-3 (Clock Structure for Host Controller) assumes Soc only supplies 50M/100M/200M, these three frequency selection, to cclk_in. So SDR12 should be 50M/2 = 25M. But for Rockchip Socs, we have RK3288_CLKGEN_DIV before cclk_in that means we don't follow this table. For DDR52-8bit, it's a mandatory requirment to use 100M as cclk_in. > > --- > > tl;dr of all the below: > > * For SD: use 180 for SDR104 to get margin, use 90 elsewhere. > * For mmc, use 180 for DDR52 to meet timings, use 180 for HS200 to get > margin, use 90 elsewhere yes, see the comments blow. > > > The nice thing about this is, at least for rk3288, things don't change > much from today where we use 90 degrees for most SD cards. > > --- > > For new patch, see: <https://patchwork.kernel.org/patch/9075141/> > > --- > > Details: > > So assuming measurements I saw from an EE are correct, measured > "delay_o_ns" is ~ .5 ns for rk3399. With that, we can do some math. Soc specific. But it's a trivial margin. > Please correct any mistakes. Math is hard. Note that on all Rockchip > devices I've seen pins are limited to 150 MHz and when we choose 150 > MHz we end up at 148.5 MHz. Calculations aren't too different if we > use 150 for that case. > > > MMC_TIMING_MMC_DDR52: > min hold time = 2.5 ns > min data delay = 2.5 ns + .5 ns = 3 ns > with 100 MHz clock, 90 degree: 2.5 ns > with 100 MHz clock, 180 degree: 5.0 ns > > ...need 180 degree > When running in DDR52-8bit, 2 cycle of cclk_in is used for a single data bit internally. When shifting cclk_in_drv to 180, we can see: cclk_in ____ ____ ____ ____ ___ __| |____| |____| |____| |____| cclk_in_drv-180 ____ ____ ____ ____ ___ __| |____| |____| |____| |____| cclk_in_for_DDR52-8bit _________ _____ __| |_________| So cclk_in_drv shifts 180 from cclk_in, which means it's just in the 90 degrees of cclk_in_for_DDR52-8bit. That is what I'd expect that when running in DDR mode, the cclk_in_drv should be in the 90 degree of *Actual Internal CLK*, not cclk_in always. But fot SD DDR50-4bit, it just use cclk_in internlly, so 90 should be okay. So we were not in the same page of what the ref-clk is. But I think now we are in. Agree on all the calculation below. > -- > > SD DDR50: > min hold time = .8 ns > min data delay = .8 ns + .5 ns = 1.3 ns > with 50 MHz clock, 90 degree: 5 ns > with 50 MHz clock, 180 degree: 10 ns > > ...90 degree is good and 180 is massive overkill. Use 90. > > -- > > SD SDR104 (limited to 148.5 MHz on existing Rockchip parts): > min hold time = .8 ns > min data delay = .8 ns + .5 ns = 1.3 ns > with 148.5 MHz clock, 90 degree: 1.68 ns > with 148.5 MHz clock, 180 degree: 3.37 ns > > ...so 90 degrees would probably work OK, but 180 degrees gives more margin. > ...probably explains why existing rk3288 devices w/ 90 degree work OK. > > if we had 200 MHz clock, 90 degree: 1.25 ns > if we had 200 MHz clock, 180 degree: 2.50 ns > > -- > > SD SDR50: > min hold time = .8 ns > min data delay = .8 ns + .5 ns = 1.3 ns > with 100 MHz clock, 90 degree: 2.5 ns > with 100 MHz clock, 180 degree: 5.0 ns > > ...90 degree is great w/ plenty of margin > > -- > > SD SDR25: > min hold time = 2 ns > min data delay = 2 ns + .5 ns = 2.5 ns > with 50 MHz clock, 90 degree: 5 ns > with 50 MHz clock, 180 degree: 10 ns > > ...90 degree is good and 180 is massive overkill. Use 90. > > -- > > SD SDR12 (databook shows cclk_in as 50 MHz here we'll get 25 MHz): > min hold time = 5 ns > min data delay = 5 ns + .5 ns = 5.5 ns > with 25 MHz clock, 90 degree: 10 ns > with 25 MHz clock, 180 degree: 20 ns > > ...90 degree is good (databook suggests 180 for this mode due to cclk_in = 50) > > -- > > ID Mode > min hold time = 5 ns > min data delay = 5 ns + .5 ns = 5.5 ns > with 400 kHz clock, 90 degree: 625 ns > with 400 kHz clock, 180 degree: 1250 ns > > ...90 degree is good (databook suggests 180 for this mode due to > cclk_in = 50) > > -- > > MMC High speed: > min hold time = 2.5 ns > min data delay = 2.5 ns + .5 ns = 3 ns > with 50 MHz clock, 90 degree: 5 ns > with 50 MHz clock, 180 degree: 10 ns > > ...90 degree is good > > -- > > HS200: > min hold time (tIH from JEDEC spec): .8 ns > ...math all works out the same as SDR104. > > -- > > HS400: > we don't support it anyway > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt index ea5614b..c48dba6 100644 --- a/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt @@ -29,6 +29,9 @@ Optional Properties: probing, low speeds or in case where all phases work at tuning time. If not specified 0 deg will be used. +* rockchip,default-drv-phase: The default phase to set ciu_drv at probing + for host to write data to devices. If not specified 180 deg will be used. + Example: rkdwmmc0@12200000 {
rockchip,default-drv-phase is used to set the default drv degrees. drv phases will decide the write timing of mmc controller. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 3 +++ 1 file changed, 3 insertions(+)