From: Steven Lee <hidden> Date: 2021-05-06 10:07:33
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 provides the example for enabling regulators and
supporting SDR104 mode on AST2600-A2 EVB.
The description of the reference design of AST2600-A2 EVB is added
in the dts file.
This patch also include a helper for updating AST2600 sdhci capability
registers, and assert/deassert the reset signal for cleaning up AST2600
eMMC controller before eMMC is probed.
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 (5):
dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
ARM: dts: aspeed: ast2600evb: Add phase correction for emmc
controller.
mmc: sdhci-of-aspeed: Add a helper for updating capability register.
mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 ++++++++++++++++-
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 18 ++-
drivers/mmc/host/sdhci-of-aspeed.c | 106 ++++++++++++++++--
3 files changed, 211 insertions(+), 14 deletions(-)
--
2.17.1
From: Steven Lee <hidden> Date: 2021-05-06 10:05:44
AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.
In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.
If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.
AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.
Signed-off-by: Steven Lee <redacted>
---
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
1 file changed, 97 insertions(+), 4 deletions(-)
From: Steven Lee <hidden> Date: 2021-05-06 10:05:56
The patch add a new function aspeed_sdc_set_slot_capability() for
updating sdhci capability register.
Signed-off-by: Steven Lee <redacted>
---
drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
From: Steven Lee <hidden> Date: 2021-05-06 10:06:12
Add the description for describing the AST2600-A2 EVB reference design of
GPIO regulators.
Signed-off-by: Steven Lee <redacted>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
From: Steven Lee <hidden> Date: 2021-05-06 10:06:22
Set MMC timing-phase register by adding the phase correction binding in the
device tree.
Signed-off-by: Steven Lee <redacted>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Steven Lee <hidden> Date: 2021-05-06 10:08:24
For cleaning up the AST2600 eMMC controller, the reset signal should be
asserted and deasserted before it is probed.
Signed-off-by: Steven Lee <redacted>
---
drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 9 deletions(-)
From: Philipp Zabel <p.zabel@pengutronix.de> Date: 2021-05-06 10:25:34
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted hunk
For cleaning up the AST2600 eMMC controller, the reset signal should be
asserted and deasserted before it is probed.
Signed-off-by: Steven Lee <redacted>
---
drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 9 deletions(-)
There is no need to initialize these variables to NULL, see below:
quoted hunk
int ret;
sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
@@ -546,6 +569,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev) spin_lock_init(&sdc->lock);+ match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
match is set unconditionally before it is used,
+ if (!match)
+ return -ENODEV;
+
+ if (match->data)
+ info = match->data;
and info could be set unconditionally as well:
info = match->data;
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
In general, I would assert/deassert the reset only after all resources
are successfully acquired. This might avoid unnecessary resets in case
of probe deferrals.
regards
Philipp
From: Rob Herring <robh@kernel.org> Date: 2021-05-07 01:13:29
On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
quoted hunk
AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.
In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.
If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.
AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.
Signed-off-by: Steven Lee <redacted>
---
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
1 file changed, 97 insertions(+), 4 deletions(-)
From: Andrew Jeffery <hidden> Date: 2021-05-07 01:33:16
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Andrew
From: Andrew Jeffery <hidden> Date: 2021-05-07 01:40:54
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
quoted hunk
Add the description for describing the AST2600-A2 EVB reference design of
GPIO regulators.
Signed-off-by: Steven Lee <redacted>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Okay, I think the comment is in the right place, but I feel this patch
should also add the regulator nodes and hook them up to the SDHCIs.
Given Rob isn't super keen on a second example in the binding document
I think you can just cut the example out and paste it in here.
Andrew
From: Andrew Jeffery <hidden> Date: 2021-05-07 02:14:16
Hi Steven,
I have some minor comments. I expect you're going to do a v4 of the
series, so if you'd like to clean them up in the process I'd appreciate
it.
However, from a pragmatic standpoint I think the patch is in good shape.
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
The patch add a new function aspeed_sdc_set_slot_capability() for
updating sdhci capability register.
The commit message should explain why the patch is necessary and not
what it does, as what it does is contained in the diff.
It's okay to explain *how* the patch acheives its goals if the
implementation is subtle or complex.
Maybe the commit message could be something like:
I prefer we don't take up so much vertical space here. I think this
could be just a couple of lines with multiple variables per line. We
can go to 100 chars per line.
The rest of the driver follows "reverse christmas tree" (longest to
shortest declaration) style, so I prefer we try to maintain consistency
where we can. Essentially, declare them in this order:
u32 mirror_reg_offset;
u32 cap_val;
u8 cap_reg;
Again here with the reverse-christmas-tree style, so:
const struct aspeed_sdhci_pdata *aspeed_pdata;
struct device_node *np = pdev->dev.of_node;
struct sdhci_pltfm_host *pltfm_host;
...
From: Steven Lee <hidden> Date: 2021-05-07 03:15:24
The 05/07/2021 09:13, Rob Herring wrote:
On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
quoted
AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.
In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.
If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.
AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.
Signed-off-by: Steven Lee <redacted>
---
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
1 file changed, 97 insertions(+), 4 deletions(-)
Why are you adding 'sdhci'. That's not useful as a compatible given how
many quirks different implementations have.
It is for passing the dtbs_check of the second example.
Without this definition, many device trees have the following
error:
['aspeed,ast2600-sdhci', 'sdhci'] is too long
Additional items are not allowed ('sdhci' was unexpected)
Regardless, I will remove it, and move the new example to device
tree.
Thanks.
The original example is for AST2500 which doesn't support UHS mode.
The new example teaches users how to enable sdhci with UHS mode, add
gpio regulators, and adjust clock phase for AST2600-A2.
I will move the new example to ast2600 device tree per the
review comment.
https://lkml.org/lkml/2021/5/6/968
From: Steven Lee <hidden> Date: 2021-05-07 03:31:35
The 05/07/2021 09:40, Andrew Jeffery wrote:
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
quoted
Add the description for describing the AST2600-A2 EVB reference design of
GPIO regulators.
Signed-off-by: Steven Lee <redacted>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Okay, I think the comment is in the right place, but I feel this patch
should also add the regulator nodes and hook them up to the SDHCIs.
Given Rob isn't super keen on a second example in the binding document
I think you can just cut the example out and paste it in here.
Hi Andrew,
Since AST2600-A0 and AST2600-A1 don't have regulators, do you mean cut
the example from dt-binding and paste it to aspeed-ast2600-evb.dts but
comment out the example?
From: Andrew Jeffery <hidden> Date: 2021-05-07 03:43:01
On Fri, 7 May 2021, at 13:00, Steven Lee wrote:
The 05/07/2021 09:40, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
quoted
Add the description for describing the AST2600-A2 EVB reference design of
GPIO regulators.
Signed-off-by: Steven Lee <redacted>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Okay, I think the comment is in the right place, but I feel this patch
should also add the regulator nodes and hook them up to the SDHCIs.
Given Rob isn't super keen on a second example in the binding document
I think you can just cut the example out and paste it in here.
Hi Andrew,
Since AST2600-A0 and AST2600-A1 don't have regulators, do you mean cut
the example from dt-binding and paste it to aspeed-ast2600-evb.dts but
comment out the example?
If the board design varies with the silicon revision we should probably
have devicetrees that are appropriate for each, so an
aspeed-ast2600-evb-a2.dts
#include "aspeed-ast2600-evb.dts" at the top and go from there.
Cheers,
Andrew
From: Steven Lee <hidden> Date: 2021-05-07 06:03:50
The 05/06/2021 18:24, Philipp Zabel wrote:
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
For cleaning up the AST2600 eMMC controller, the reset signal should be
asserted and deasserted before it is probed.
Signed-off-by: Steven Lee <redacted>
---
drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 9 deletions(-)
There is no need to initialize these variables to NULL, see below:
Will modify it.
quoted
int ret;
sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
@@ -546,6 +569,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev) spin_lock_init(&sdc->lock);+ match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
match is set unconditionally before it is used,
quoted
+ if (!match)
+ return -ENODEV;
+
+ if (match->data)
+ info = match->data;
and info could be set unconditionally as well:
info = match->data;
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
Will modify as you suggest.
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
In general, I would assert/deassert the reset only after all resources
are successfully acquired. This might avoid unnecessary resets in case
of probe deferrals.
Thanks for the suggestion. I will try to move the implementation of
reset after devm_ioremap_resource().
From: Steven Lee <hidden> Date: 2021-05-07 06:25:35
The 05/07/2021 09:32, Andrew Jeffery wrote:
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
From: Steven Lee <hidden> Date: 2021-05-07 07:00:38
The 05/07/2021 10:13, Andrew Jeffery wrote:
Hi Steven,
I have some minor comments. I expect you're going to do a v4 of the
series, so if you'd like to clean them up in the process I'd appreciate
it.
Yes, I am going to prepare v4 patch for meeting reviewer's expectation
including your comment in this patch.
I've learned a lot from your suggestion for driver upstream.
Many thanks!
However, from a pragmatic standpoint I think the patch is in good shape.
On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
quoted
The patch add a new function aspeed_sdc_set_slot_capability() for
updating sdhci capability register.
The commit message should explain why the patch is necessary and not
what it does, as what it does is contained in the diff.
It's okay to explain *how* the patch acheives its goals if the
implementation is subtle or complex.
Maybe the commit message could be something like:
I prefer we don't take up so much vertical space here. I think this
could be just a couple of lines with multiple variables per line. We
can go to 100 chars per line.
I will change the function as the follows:
static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc,
int capability, bool enable, u8 slot)
The rest of the driver follows "reverse christmas tree" (longest to
shortest declaration) style, so I prefer we try to maintain consistency
where we can. Essentially, declare them in this order:
u32 mirror_reg_offset;
u32 cap_val;
u8 cap_reg;
Again here with the reverse-christmas-tree style, so:
const struct aspeed_sdhci_pdata *aspeed_pdata;
struct device_node *np = pdev->dev.of_node;
struct sdhci_pltfm_host *pltfm_host;
...
From: Andrew Jeffery <hidden> Date: 2021-05-07 07:39:07
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
From: Rob Herring <robh@kernel.org> Date: 2021-05-07 17:22:01
On Thu, May 6, 2021 at 10:14 PM Steven Lee [off-list ref] wrote:
The 05/07/2021 09:13, Rob Herring wrote:
quoted
On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
quoted
AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.
In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.
If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.
AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.
Signed-off-by: Steven Lee <redacted>
---
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
1 file changed, 97 insertions(+), 4 deletions(-)
Why are you adding 'sdhci'. That's not useful as a compatible given how
many quirks different implementations have.
It is for passing the dtbs_check of the second example.
Without this definition, many device trees have the following
error:
['aspeed,ast2600-sdhci', 'sdhci'] is too long
Additional items are not allowed ('sdhci' was unexpected)
I would probably fix the dts files then. Does anything depend on 'sdhci'?
Rob
From: Steven Lee <hidden> Date: 2021-05-10 02:34:08
The 05/08/2021 01:21, Rob Herring wrote:
On Thu, May 6, 2021 at 10:14 PM Steven Lee [off-list ref] wrote:
quoted
The 05/07/2021 09:13, Rob Herring wrote:
quoted
On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
quoted
AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.
In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.
If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.
AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.
Signed-off-by: Steven Lee <redacted>
---
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 +++++++++++++++++-
1 file changed, 97 insertions(+), 4 deletions(-)
Why are you adding 'sdhci'. That's not useful as a compatible given how
many quirks different implementations have.
It is for passing the dtbs_check of the second example.
Without this definition, many device trees have the following
error:
['aspeed,ast2600-sdhci', 'sdhci'] is too long
Additional items are not allowed ('sdhci' was unexpected)
I would probably fix the dts files then. Does anything depend on 'sdhci'?
From: Steven Lee <hidden> Date: 2021-05-10 06:04:58
The 05/07/2021 15:36, Andrew Jeffery wrote:
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
quoted
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
From: Andrew Jeffery <hidden> Date: 2021-05-13 00:46:45
On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
The 05/07/2021 15:36, Andrew Jeffery wrote:
quoted
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
quoted
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
Okay, so what's the issue that the patch addresses? Is there a bug?
Presumably if u-boot isn't making use of the eMMC the clock won't be
on, so we'll do the reset if the kernel wants to make use of the
device. If u-boot _is_ using the eMMC, u-boot will have done the
correct clock enable/reset sequence and so the controller should be
ready to go?
The only potential issue remaining is u-boot leaving the controller in
a configuration the kernel isn't expecting when handing over. If that's
the issue then we've forgotten to do some specific initialisation (i.e.
not just reset the entire thing) of the controller in the driver probe
path, right?
FWIW I haven't recently seen any poor behaviour from the controller or
driver. For us (IBM) it seems to be working well since we sorted out
the phase configuration.
Andrew
From: Steven Lee <hidden> Date: 2021-05-14 02:10:51
The 05/13/2021 08:42, Andrew Jeffery wrote:
On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
quoted
The 05/07/2021 15:36, Andrew Jeffery wrote:
quoted
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
quoted
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
Okay, so what's the issue that the patch addresses? Is there a bug?
Presumably if u-boot isn't making use of the eMMC the clock won't be
on, so we'll do the reset if the kernel wants to make use of the
device. If u-boot _is_ using the eMMC, u-boot will have done the
correct clock enable/reset sequence and so the controller should be
ready to go?
The only potential issue remaining is u-boot leaving the controller in
a configuration the kernel isn't expecting when handing over. If that's
the issue then we've forgotten to do some specific initialisation (i.e.
not just reset the entire thing) of the controller in the driver probe
path, right?
If DMA engine is used before probing eMMC in kernel stage,
eMMC controller may have unexpected behavior when re-exectuing
identifying process.
Thus, we need to reset at the beginning of kernel since
kernel is a new stage. We should not assume some one do something
before.
FWIW I haven't recently seen any poor behaviour from the controller or
driver. For us (IBM) it seems to be working well since we sorted out
the phase configuration.
Yes, you are right, everything work well currently. But, kernel is a new
stage, we cannot assume eMMC controller is at initial state when
entering kernel stage.
From: Andrew Jeffery <hidden> Date: 2021-05-14 02:37:57
On Fri, 14 May 2021, at 11:39, Steven Lee wrote:
The 05/13/2021 08:42, Andrew Jeffery wrote:
quoted
On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
quoted
The 05/07/2021 15:36, Andrew Jeffery wrote:
quoted
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
quoted
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
Okay, so what's the issue that the patch addresses? Is there a bug?
Presumably if u-boot isn't making use of the eMMC the clock won't be
on, so we'll do the reset if the kernel wants to make use of the
device. If u-boot _is_ using the eMMC, u-boot will have done the
correct clock enable/reset sequence and so the controller should be
ready to go?
The only potential issue remaining is u-boot leaving the controller in
a configuration the kernel isn't expecting when handing over. If that's
the issue then we've forgotten to do some specific initialisation (i.e.
not just reset the entire thing) of the controller in the driver probe
path, right?
If DMA engine is used before probing eMMC in kernel stage,
eMMC controller may have unexpected behavior when re-exectuing
identifying process.
Thus, we need to reset at the beginning of kernel since
kernel is a new stage. We should not assume some one do something
before.
quoted
FWIW I haven't recently seen any poor behaviour from the controller or
driver. For us (IBM) it seems to be working well since we sorted out
the phase configuration.
Yes, you are right, everything work well currently. But, kernel is a new
stage, we cannot assume eMMC controller is at initial state when
entering kernel stage.
Okay. That sounds true no matter what the hardware design though (going
back to the difference between the 2400/2500 and 2600).
Given the reset is tied up in the clock gating, it would be nice if we
could do the following in aspeed_sdc_probe():
/* Clean up the controller in case it wasn't left in a good state by the bootloader */clock_disable_unprepare(...);clock_prepare_enable(...);
But the enable_count tracked by clock_core_{en,dis}able() kills that
idea.
This makes it seem like we need to break out the appropriate indexes
to add `resets` properties in the devicetree. This will need some input
from Joel, given the eMMC/SD resets can't currently be handled that way.
Andrew
From: Steven Lee <hidden> Date: 2021-05-19 11:00:01
The 05/14/2021 10:37, Andrew Jeffery wrote:
On Fri, 14 May 2021, at 11:39, Steven Lee wrote:
quoted
The 05/13/2021 08:42, Andrew Jeffery wrote:
quoted
On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
quoted
The 05/07/2021 15:36, Andrew Jeffery wrote:
quoted
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
quoted
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
Okay, so what's the issue that the patch addresses? Is there a bug?
Presumably if u-boot isn't making use of the eMMC the clock won't be
on, so we'll do the reset if the kernel wants to make use of the
device. If u-boot _is_ using the eMMC, u-boot will have done the
correct clock enable/reset sequence and so the controller should be
ready to go?
The only potential issue remaining is u-boot leaving the controller in
a configuration the kernel isn't expecting when handing over. If that's
the issue then we've forgotten to do some specific initialisation (i.e.
not just reset the entire thing) of the controller in the driver probe
path, right?
If DMA engine is used before probing eMMC in kernel stage,
eMMC controller may have unexpected behavior when re-exectuing
identifying process.
Thus, we need to reset at the beginning of kernel since
kernel is a new stage. We should not assume some one do something
before.
quoted
FWIW I haven't recently seen any poor behaviour from the controller or
driver. For us (IBM) it seems to be working well since we sorted out
the phase configuration.
Yes, you are right, everything work well currently. But, kernel is a new
stage, we cannot assume eMMC controller is at initial state when
entering kernel stage.
Okay. That sounds true no matter what the hardware design though (going
back to the difference between the 2400/2500 and 2600).
Given the reset is tied up in the clock gating, it would be nice if we
could do the following in aspeed_sdc_probe():
/* Clean up the controller in case it wasn't left in a good state by the bootloader */clock_disable_unprepare(...);clock_prepare_enable(...);
But the enable_count tracked by clock_core_{en,dis}able() kills that
idea.
This makes it seem like we need to break out the appropriate indexes
to add `resets` properties in the devicetree. This will need some input
from Joel, given the eMMC/SD resets can't currently be handled that way.
Hi Adnrew,
I was wondering if I should wait for Joel's comment, or
1. Drop this patch in this patch series, and prepare another patch
series for this issue in the future. Since sdhci works well as long as we
don't use DMA engine before kernel stage.
2. Modify the reset control according to Philipp and your comment.
Thanks and look forward to having your suggestion.
Steven
From: Andrew Jeffery <hidden> Date: 2021-05-19 23:09:56
On Wed, 19 May 2021, at 20:27, Steven Lee wrote:
The 05/14/2021 10:37, Andrew Jeffery wrote:
quoted
On Fri, 14 May 2021, at 11:39, Steven Lee wrote:
quoted
The 05/13/2021 08:42, Andrew Jeffery wrote:
quoted
On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
quoted
The 05/07/2021 15:36, Andrew Jeffery wrote:
quoted
On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
quoted
The 05/07/2021 09:32, Andrew Jeffery wrote:
quoted
On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
quoted
Hi Steven,
On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
quoted
+ if (info) {
+ if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+ sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().
quoted
+ if (!IS_ERR(sdc->rst)) {
Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?
It feels like we should get rid of the flag and leave it to the
devicetree.
Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?
quoted
I'm still kind of surprised it's not something we want to do for the
2400 and 2500 as well.
Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.
Okay, so what's the issue that the patch addresses? Is there a bug?
Presumably if u-boot isn't making use of the eMMC the clock won't be
on, so we'll do the reset if the kernel wants to make use of the
device. If u-boot _is_ using the eMMC, u-boot will have done the
correct clock enable/reset sequence and so the controller should be
ready to go?
The only potential issue remaining is u-boot leaving the controller in
a configuration the kernel isn't expecting when handing over. If that's
the issue then we've forgotten to do some specific initialisation (i.e.
not just reset the entire thing) of the controller in the driver probe
path, right?
If DMA engine is used before probing eMMC in kernel stage,
eMMC controller may have unexpected behavior when re-exectuing
identifying process.
Thus, we need to reset at the beginning of kernel since
kernel is a new stage. We should not assume some one do something
before.
quoted
FWIW I haven't recently seen any poor behaviour from the controller or
driver. For us (IBM) it seems to be working well since we sorted out
the phase configuration.
Yes, you are right, everything work well currently. But, kernel is a new
stage, we cannot assume eMMC controller is at initial state when
entering kernel stage.
Okay. That sounds true no matter what the hardware design though (going
back to the difference between the 2400/2500 and 2600).
Given the reset is tied up in the clock gating, it would be nice if we
could do the following in aspeed_sdc_probe():
/* Clean up the controller in case it wasn't left in a good state by the bootloader */clock_disable_unprepare(...);clock_prepare_enable(...);
But the enable_count tracked by clock_core_{en,dis}able() kills that
idea.
This makes it seem like we need to break out the appropriate indexes
to add `resets` properties in the devicetree. This will need some input
from Joel, given the eMMC/SD resets can't currently be handled that way.
Hi Adnrew,
I was wondering if I should wait for Joel's comment, or
1. Drop this patch in this patch series, and prepare another patch
series for this issue in the future. Since sdhci works well as long as we
don't use DMA engine before kernel stage.
I think this is the way forward, it's kind of separate to what you're
trying to achieve with the rest of the patches in this series.
I'll poke Joel.
Cheers,
Andrew