| Message ID | 20210524073308.9328-1-steven_lee@aspeedtech.com |
|---|---|
| Headers | show |
| Series | mmc: sdhci-of-aspeed: Support toggling SD bus signal | expand |
On Mon, 24 May 2021, at 17:02, Steven Lee wrote: > AST2600 A2(or newer) EVB has gpio regulators for toggling signal voltage > between 3.3v and 1.8v, the patch adds sdhci node and gpio regulator in the > dts file and adds comment for describing the reference design. > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> Acked-by: Andrew Jeffery <andrew@aj.id.au>
On Mon, 24 May 2021, at 17:02, Steven Lee wrote: > aspeed-ast2600-evb.dts was modified for supporting A2 evb. > Since A1/A0 evbs don't have GPIO regulators and SD clock frequency(SCU210) > is different to A2 as well. Adding a new dts that removes new nodes > created in aspeed-ast2600-evb.dts is necessary. > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> Acked-by: Andrew Jeffery <andrew@aj.id.au>
The 05/25/2021 15:55, Joel Stanley wrote: > On Mon, 24 May 2021 at 07:33, Steven Lee <steven_lee@aspeedtech.com> wrote: > > > > AST2600-A2 EVB has the reference design for enabling SD bus > > power and toggling SD bus signal voltage between 3.3v and 1.8v by > > GPIO regulators. > > This patch series adds sdhci node and gpio regulators in a new dts file > > for AST2600-A2 EVB. > > The description of the reference design of AST2600-A2 EVB is added > > in the new dts file. > > > > This patch also include a helper for updating AST2600 sdhci capability > > registers. > > The device trees look good: > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > I've applied patches 1-3 to the aspeed tree for v5.14. I made a little > fix to patch 3 as it needed to add the new device tree to the > makefile. > Thanks! > When I was testing on my A2 EVB I saw this: > > [ 1.436219] sdhci-aspeed 1e750100.sdhci: Requested out of range > phase tap 192 for 9 degrees of phase compensation at 1562500Hz, > clamping to tap 15 > [ 1.450913] sdhci-aspeed 1e750100.sdhci: Requested out of range > phase tap 963 for 45 degrees of phase compensation at 1562500Hz, > clamping to tap 15 > > Do you know what is happening there? > Per MMC spec, eMMC bus speed is set as legacy mode(0~26MHz) at startup of eMMC initializtion flow. Clock phase calculation is triggered in set_clock() and it calculates taps based on phase_deg(<9>, <225>) in the dts file and the current speed(1562500Hz), which causes the warning message you mentioned. As the phase_deg in the dts file should be calculated with 100MHz. https://lkml.org/lkml/2021/5/24/95 But after some initialization flow, eMMC bus speed will be set to correct speed(100MHz). Clock phase calculation will be triggered again to get correct taps. > Cheers, > > Joel
On Tue, 25 May 2021, at 22:26, Joel Stanley wrote: > On Tue, 25 May 2021 at 09:48, Steven Lee <steven_lee@aspeedtech.com> wrote: > > > > The 05/25/2021 15:55, Joel Stanley wrote: > > > When I was testing on my A2 EVB I saw this: > > > > > > [ 1.436219] sdhci-aspeed 1e750100.sdhci: Requested out of range > > > phase tap 192 for 9 degrees of phase compensation at 1562500Hz, > > > clamping to tap 15 > > > [ 1.450913] sdhci-aspeed 1e750100.sdhci: Requested out of range > > > phase tap 963 for 45 degrees of phase compensation at 1562500Hz, > > > clamping to tap 15 > > > > > > Do you know what is happening there? > > > > > > > Per MMC spec, eMMC bus speed is set as legacy mode(0~26MHz) at startup of > > eMMC initializtion flow. Clock phase calculation is triggered in set_clock() > > and it calculates taps based on phase_deg(<9>, <225>) in the dts file and the > > current speed(1562500Hz), which causes the warning message you mentioned. > > As the phase_deg in the dts file should be calculated with 100MHz. > > > > https://lkml.org/lkml/2021/5/24/95 > > > > But after some initialization flow, eMMC bus speed will be set to > > correct speed(100MHz). > > Clock phase calculation will be triggered again to get correct taps. > > Thanks for the explanation. I added another debug print and I can see > it doing what you describe: > > [ 1.465904] sdhci-aspeed 1e750100.sdhci: Requested out of range > phase tap 192 for 9 degrees of phase compensation at 1562500Hz, > clamping to tap 15 > [ 1.480598] sdhci-aspeed 1e750100.sdhci: rate 1562500 phase 9 tap 15 > [ 1.490316] sdhci-aspeed 1e750100.sdhci: Requested out of range > phase tap 963 for 45 degrees of phase compensation at 1562500Hz, > clamping to tap 15 > [ 1.505077] sdhci-aspeed 1e750100.sdhci: rate 1562500 phase 45 tap 15 > [ 1.515059] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 9 tap 3 > [ 1.524886] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 45 tap 15 > [ 1.534904] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 9 tap 3 > [ 1.544713] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 45 tap 15 > > We should change the "out of range" message to be dev_dbg, as it is > expected on a normal boot. I would think the issue is rather that we shouldn't be applying a phase correction for a bus speed that isn't what the correction was specified for. Let me look at this a bit further. Andrew
On Wed, 26 May 2021, at 08:29, Andrew Jeffery wrote: > > > On Tue, 25 May 2021, at 22:26, Joel Stanley wrote: > > On Tue, 25 May 2021 at 09:48, Steven Lee <steven_lee@aspeedtech.com> wrote: > > > > > > The 05/25/2021 15:55, Joel Stanley wrote: > > > > When I was testing on my A2 EVB I saw this: > > > > > > > > [ 1.436219] sdhci-aspeed 1e750100.sdhci: Requested out of range > > > > phase tap 192 for 9 degrees of phase compensation at 1562500Hz, > > > > clamping to tap 15 > > > > [ 1.450913] sdhci-aspeed 1e750100.sdhci: Requested out of range > > > > phase tap 963 for 45 degrees of phase compensation at 1562500Hz, > > > > clamping to tap 15 > > > > > > > > Do you know what is happening there? > > > > > > > > > > Per MMC spec, eMMC bus speed is set as legacy mode(0~26MHz) at startup of > > > eMMC initializtion flow. Clock phase calculation is triggered in set_clock() > > > and it calculates taps based on phase_deg(<9>, <225>) in the dts file and the > > > current speed(1562500Hz), which causes the warning message you mentioned. > > > As the phase_deg in the dts file should be calculated with 100MHz. > > > > > > https://lkml.org/lkml/2021/5/24/95 > > > > > > But after some initialization flow, eMMC bus speed will be set to > > > correct speed(100MHz). > > > Clock phase calculation will be triggered again to get correct taps. > > > > Thanks for the explanation. I added another debug print and I can see > > it doing what you describe: > > > > [ 1.465904] sdhci-aspeed 1e750100.sdhci: Requested out of range > > phase tap 192 for 9 degrees of phase compensation at 1562500Hz, > > clamping to tap 15 > > [ 1.480598] sdhci-aspeed 1e750100.sdhci: rate 1562500 phase 9 tap 15 > > [ 1.490316] sdhci-aspeed 1e750100.sdhci: Requested out of range > > phase tap 963 for 45 degrees of phase compensation at 1562500Hz, > > clamping to tap 15 > > [ 1.505077] sdhci-aspeed 1e750100.sdhci: rate 1562500 phase 45 tap 15 > > [ 1.515059] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 9 tap 3 > > [ 1.524886] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 45 tap 15 > > [ 1.534904] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 9 tap 3 > > [ 1.544713] sdhci-aspeed 1e750100.sdhci: rate 100000000 phase 45 tap 15 > > > > We should change the "out of range" message to be dev_dbg, as it is > > expected on a normal boot. > > I would think the issue is rather that we shouldn't be applying a phase > correction for a bus speed that isn't what the correction was specified > for. > > Let me look at this a bit further. Okay, looks like the issue is that setting the card timing and the bus frequency are not atomic in the sense that the bus clock enable is toggled in between the two, and the MMC core calls through the driver's set_clock() callback when toggling the bus clock state. This means the driver observes a transient state where the card timing is not aligned with the bus frequency, and so we have garbage inputs to the phase correction calculation. Ideally there'd be a better solution than just switching to dev_dbg(), but I'm not sure what it would look like. Andrew
AST2600-A2 EVB has the reference design for enabling SD bus power and toggling SD bus signal voltage between 3.3v and 1.8v by GPIO regulators. This patch series adds sdhci node and gpio regulators in a new dts file for AST2600-A2 EVB. The description of the reference design of AST2600-A2 EVB is added in the new dts file. This patch also include a helper for updating AST2600 sdhci capability registers. Changes from v4: * Move sdhci node and gpio regulator from aspeed-ast2600-evb-a2.dts to aspeed-ast2600-evb.dts. Now aspeed-ast2600-evb.dts only supports A2(or newer) evbs. * Remove aspeed-ast2600-evb-a2.dts since sdhci nodes were moved to aspeed-ast2600-evb.dts. * Add aspeed-ast2600-evb-a1.dts for A1 and A0 evbs. Changes from v3: * Remove the example of gpio regulator from dt-bindings. * Add sdhci node and gpio regulators to a new dts file. * Move the comment of the reference design to the new dts file. * Modify commit message of sdhci-of-aspeed.c. * Fix coding style issues of sdhci-of-aspeed.c. * Remove the implementation of eMMC resetc since it has no relevance to the goal that this patch series want to achieve and it may needs further discussion about the design of reset behavior. Changes from v2: * Move the comment of the reference design from dt-bindings to device tree. * Add clk-phase binding for eMMC controller. * Reimplement aspeed_sdc_set_slot_capability(). * Separate the implementation of eMMC reset to another patch file. * Fix yaml document error per the report of dt_binding_check and dtbs_check. Changes from v1: * Add the device tree example for AST2600 A2 EVB in dt-bindings document * Add timing-phase for eMMC controller. * Remove power-gpio and power-switch-gpio from sdhci driver, they should be handled by regulator. * Add a helper to update capability registers in the driver. * Sync sdhci settings from device tree to SoC capability registers. * Sync timing-phase from device tree to SoC Clock Phase Control register Please help to review. Regards, Steven Steven Lee (4): ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2 evb. ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller. ARM: dts: aspeed: ast2600evb: Add dts file for A1 and A0. mmc: sdhci-of-aspeed: Configure the SDHCIs as specified by the devicetree. arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts | 15 ++++ arch/arm/boot/dts/aspeed-ast2600-evb.dts | 87 ++++++++++++++++++++- drivers/mmc/host/sdhci-of-aspeed.c | 48 ++++++++++++ 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts