| Message ID | 20210506100312.1638-6-steven_lee@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdhci-of-aspeed: Support toggling SD bus signal | expand |
Hi Steven, On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote: > For cleaning up the AST2600 eMMC controller, the reset signal should be > asserted and deasserted before it is probed. > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> > --- > drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------ > 1 file changed, 40 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > index 4979f98ffb52..8ef06f32abff 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c [...] > @@ -533,11 +545,22 @@ static struct platform_driver aspeed_sdhci_driver = { > .remove = aspeed_sdhci_remove, > }; > > +static const struct of_device_id aspeed_sdc_of_match[] = { > + { .compatible = "aspeed,ast2400-sd-controller", }, > + { .compatible = "aspeed,ast2500-sd-controller", }, > + { .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info}, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); > + > static int aspeed_sdc_probe(struct platform_device *pdev) > > { > struct device_node *parent, *child; > struct aspeed_sdc *sdc; > + const struct of_device_id *match = NULL; > + const struct aspeed_sdc_info *info = NULL; There is no need to initialize these variables to NULL, see below: > 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? > + reset_control_assert(sdc->rst); > + reset_control_deassert(sdc->rst); Is there no need for delays between assertion and deassertion or after the reset is deasserted? > + } > + } > + } > + > sdc->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(sdc->clk)) > return PTR_ERR(sdc->clk); 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
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: > > + 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? 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
The 05/06/2021 18:24, Philipp Zabel wrote: > Hi Steven, > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote: > > For cleaning up the AST2600 eMMC controller, the reset signal should be > > asserted and deasserted before it is probed. > > > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> > > --- > > drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------ > > 1 file changed, 40 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > > index 4979f98ffb52..8ef06f32abff 100644 > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > [...] > > @@ -533,11 +545,22 @@ static struct platform_driver aspeed_sdhci_driver = { > > .remove = aspeed_sdhci_remove, > > }; > > > > +static const struct of_device_id aspeed_sdc_of_match[] = { > > + { .compatible = "aspeed,ast2400-sd-controller", }, > > + { .compatible = "aspeed,ast2500-sd-controller", }, > > + { .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info}, > > + { } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); > > + > > static int aspeed_sdc_probe(struct platform_device *pdev) > > > > { > > struct device_node *parent, *child; > > struct aspeed_sdc *sdc; > > + const struct of_device_id *match = NULL; > > + const struct aspeed_sdc_info *info = NULL; > > There is no need to initialize these variables to NULL, see below: > Will modify it. > > 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(). > Will modify as you suggest. > > + 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? > Will return error here. > > + reset_control_assert(sdc->rst); > > + reset_control_deassert(sdc->rst); > > Is there no need for delays between assertion and deassertion or after > the reset is deasserted? > Per the internal discussion, I Will add udelay(1). > > + } > > + } > > + } > > + > > sdc->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(sdc->clk)) > > return PTR_ERR(sdc->clk); > > 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(). > regards > Philipp
The 05/07/2021 09:32, Andrew Jeffery wrote: > > > 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: > > > + 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? > > 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. > Andrew
On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > 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: > > > > + 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? > > > > 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. So digging a bit more deeply on this, it looks like the reset is already taken care of by drivers/clk/clk-ast2600.c in the clk_prepare_enable() path. clk-ast2600 handles resets when enabling the clock for most peripherals: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 and this is true for both the SD controller and the eMMC controller: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 If this weren't the case you'd specify a reset property in the SD/eMMC devicetree nodes for the 2600 and then use devm_reset_control_get_optional_exclusive() as Philipp suggested. See the reset binding here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 So on the surface it seems the reset handling in this patch is unnecessary. Have you observed an issue with the SoC that means it's required? Andrew
The 05/07/2021 15:36, Andrew Jeffery wrote: > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > 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: > > > > > + 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? > > > > > > 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. > > So digging a bit more deeply on this, it looks like the reset is > already taken care of by drivers/clk/clk-ast2600.c in the > clk_prepare_enable() path. > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > and this is true for both the SD controller and the eMMC controller: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > If this weren't the case you'd specify a reset property in the SD/eMMC > devicetree nodes for the 2600 and then use > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > the reset binding here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > So on the surface it seems the reset handling in this patch is > unnecessary. Have you observed an issue with the SoC that means it's > required? > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), aspeed_g6_clk_enable() does reset eMMC. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 However, the clock of eMMC is enabled in my u-boot(2019.04). So it is retruned in the condition of aspeed_g6_clk_is_enabled() below and doesn't reset eMMC. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 > Andrew
On Mon, 10 May 2021, at 15:33, Steven Lee wrote: > The 05/07/2021 15:36, Andrew Jeffery wrote: > > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > > > > 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: > > > > > > + 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? > > > > > > > > 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. > > > > So digging a bit more deeply on this, it looks like the reset is > > already taken care of by drivers/clk/clk-ast2600.c in the > > clk_prepare_enable() path. > > > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > > > and this is true for both the SD controller and the eMMC controller: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > > > If this weren't the case you'd specify a reset property in the SD/eMMC > > devicetree nodes for the 2600 and then use > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > > the reset binding here: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > > > So on the surface it seems the reset handling in this patch is > > unnecessary. Have you observed an issue with the SoC that means it's > > required? > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), > aspeed_g6_clk_enable() does reset eMMC. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 > > However, the clock of eMMC is enabled in my u-boot(2019.04). > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below > and doesn't reset eMMC. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 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
The 05/13/2021 08:42, Andrew Jeffery wrote: > > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote: > > The 05/07/2021 15:36, Andrew Jeffery wrote: > > > > > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > > > > > > > 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: > > > > > > > + 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? > > > > > > > > > > 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. > > > > > > So digging a bit more deeply on this, it looks like the reset is > > > already taken care of by drivers/clk/clk-ast2600.c in the > > > clk_prepare_enable() path. > > > > > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > > > > > and this is true for both the SD controller and the eMMC controller: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > > > > > If this weren't the case you'd specify a reset property in the SD/eMMC > > > devicetree nodes for the 2600 and then use > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > > > the reset binding here: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > > > > > So on the surface it seems the reset handling in this patch is > > > unnecessary. Have you observed an issue with the SoC that means it's > > > required? > > > > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), > > aspeed_g6_clk_enable() does reset eMMC. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 > > > > However, the clock of eMMC is enabled in my u-boot(2019.04). > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below > > and doesn't reset eMMC. > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 > > 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. > Andrew
On Fri, 14 May 2021, at 11:39, Steven Lee wrote: > The 05/13/2021 08:42, Andrew Jeffery wrote: > > > > > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote: > > > The 05/07/2021 15:36, Andrew Jeffery wrote: > > > > > > > > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > > > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > > > > > > > > > > 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: > > > > > > > > + 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? > > > > > > > > > > > > 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. > > > > > > > > So digging a bit more deeply on this, it looks like the reset is > > > > already taken care of by drivers/clk/clk-ast2600.c in the > > > > clk_prepare_enable() path. > > > > > > > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > > > > > > > and this is true for both the SD controller and the eMMC controller: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > > > > > > > If this weren't the case you'd specify a reset property in the SD/eMMC > > > > devicetree nodes for the 2600 and then use > > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > > > > the reset binding here: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > > > > > > > So on the surface it seems the reset handling in this patch is > > > > unnecessary. Have you observed an issue with the SoC that means it's > > > > required? > > > > > > > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), > > > aspeed_g6_clk_enable() does reset eMMC. > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 > > > > > > However, the clock of eMMC is enabled in my u-boot(2019.04). > > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below > > > and doesn't reset eMMC. > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 > > > > 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. 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
The 05/14/2021 10:37, Andrew Jeffery wrote: > > > On Fri, 14 May 2021, at 11:39, Steven Lee wrote: > > The 05/13/2021 08:42, Andrew Jeffery wrote: > > > > > > > > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote: > > > > The 05/07/2021 15:36, Andrew Jeffery wrote: > > > > > > > > > > > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > > > > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > > > > > > > > > > > > > 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: > > > > > > > > > + 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? > > > > > > > > > > > > > > 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. > > > > > > > > > > So digging a bit more deeply on this, it looks like the reset is > > > > > already taken care of by drivers/clk/clk-ast2600.c in the > > > > > clk_prepare_enable() path. > > > > > > > > > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > > > > > > > > > and this is true for both the SD controller and the eMMC controller: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > > > > > > > > > If this weren't the case you'd specify a reset property in the SD/eMMC > > > > > devicetree nodes for the 2600 and then use > > > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > > > > > the reset binding here: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > > > > > > > > > So on the surface it seems the reset handling in this patch is > > > > > unnecessary. Have you observed an issue with the SoC that means it's > > > > > required? > > > > > > > > > > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), > > > > aspeed_g6_clk_enable() does reset eMMC. > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 > > > > > > > > However, the clock of eMMC is enabled in my u-boot(2019.04). > > > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below > > > > and doesn't reset eMMC. > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 > > > > > > 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. > > 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 > Andrew
On Wed, 19 May 2021, at 20:27, Steven Lee wrote: > The 05/14/2021 10:37, Andrew Jeffery wrote: > > > > > > On Fri, 14 May 2021, at 11:39, Steven Lee wrote: > > > The 05/13/2021 08:42, Andrew Jeffery wrote: > > > > > > > > > > > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote: > > > > > The 05/07/2021 15:36, Andrew Jeffery wrote: > > > > > > > > > > > > > > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote: > > > > > > > The 05/07/2021 09:32, Andrew Jeffery wrote: > > > > > > > > > > > > > > > > > > > > > > > > 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: > > > > > > > > > > + 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? > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > So digging a bit more deeply on this, it looks like the reset is > > > > > > already taken care of by drivers/clk/clk-ast2600.c in the > > > > > > clk_prepare_enable() path. > > > > > > > > > > > > clk-ast2600 handles resets when enabling the clock for most peripherals: > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276 > > > > > > > > > > > > and this is true for both the SD controller and the eMMC controller: > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94 > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88 > > > > > > > > > > > > If this weren't the case you'd specify a reset property in the SD/eMMC > > > > > > devicetree nodes for the 2600 and then use > > > > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See > > > > > > the reset binding here: > > > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12 > > > > > > > > > > > > So on the surface it seems the reset handling in this patch is > > > > > > unnecessary. Have you observed an issue with the SoC that means it's > > > > > > required? > > > > > > > > > > > > > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(), > > > > > aspeed_g6_clk_enable() does reset eMMC. > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496 > > > > > > > > > > However, the clock of eMMC is enabled in my u-boot(2019.04). > > > > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below > > > > > and doesn't reset eMMC. > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285 > > > > > > > > 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. > > > > 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
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c index 4979f98ffb52..8ef06f32abff 100644 --- a/drivers/mmc/host/sdhci-of-aspeed.c +++ b/drivers/mmc/host/sdhci-of-aspeed.c @@ -13,6 +13,7 @@ #include <linux/of.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/reset.h> #include <linux/spinlock.h> #include "sdhci-pltfm.h" @@ -36,9 +37,16 @@ /* SDIO{14,24} */ #define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1) +#define PROBE_AFTER_ASSET_DEASSERT 0x1 + +struct aspeed_sdc_info { + u32 flag; +}; + struct aspeed_sdc { struct clk *clk; struct resource *res; + struct reset_control *rst; spinlock_t lock; void __iomem *regs; @@ -78,6 +86,10 @@ struct aspeed_sdhci { }; +struct aspeed_sdc_info ast2600_sdc_info = { + .flag = PROBE_AFTER_ASSET_DEASSERT +}; + /* * The function sets the mirror register for updating * capbilities of the current slot. @@ -533,11 +545,22 @@ static struct platform_driver aspeed_sdhci_driver = { .remove = aspeed_sdhci_remove, }; +static const struct of_device_id aspeed_sdc_of_match[] = { + { .compatible = "aspeed,ast2400-sd-controller", }, + { .compatible = "aspeed,ast2500-sd-controller", }, + { .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info}, + { } +}; + +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); + static int aspeed_sdc_probe(struct platform_device *pdev) { struct device_node *parent, *child; struct aspeed_sdc *sdc; + const struct of_device_id *match = NULL; + const struct aspeed_sdc_info *info = NULL; 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); + if (!match) + return -ENODEV; + + if (match->data) + info = match->data; + + if (info) { + if (info->flag & PROBE_AFTER_ASSET_DEASSERT) { + sdc->rst = devm_reset_control_get(&pdev->dev, NULL); + if (!IS_ERR(sdc->rst)) { + reset_control_assert(sdc->rst); + reset_control_deassert(sdc->rst); + } + } + } + sdc->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(sdc->clk)) return PTR_ERR(sdc->clk); @@ -593,15 +633,6 @@ static int aspeed_sdc_remove(struct platform_device *pdev) return 0; } -static const struct of_device_id aspeed_sdc_of_match[] = { - { .compatible = "aspeed,ast2400-sd-controller", }, - { .compatible = "aspeed,ast2500-sd-controller", }, - { .compatible = "aspeed,ast2600-sd-controller", }, - { } -}; - -MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); - static struct platform_driver aspeed_sdc_driver = { .driver = { .name = "sd-controller-aspeed",
For cleaning up the AST2600 eMMC controller, the reset signal should be asserted and deasserted before it is probed. Signed-off-by: Steven Lee <steven_lee@aspeedtech.com> --- drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 9 deletions(-)