| Message ID | 20210605173546.4102455-1-mnhagan88@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/3] net: stmmac: explicitly deassert GMAC_AHB_RESET | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 16 of 16 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 22 this patch: 22 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Lines should not end with a '(' WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 22 this patch: 22 |
| netdev/header_inline | success | Link |
On Sat 05 Jun 12:35 CDT 2021, Matthew Hagan wrote: > We are currently assuming that GMAC_AHB_RESET will already be deasserted > by the bootloader. However if this has not been done, probing of the GMAC > will fail. To remedy this we must ensure GMAC_AHB_RESET has been deasserted > prior to probing. > Sounds good, just some small style comments below. > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 7 +++++++ > include/linux/stmmac.h | 1 + > 3 files changed, 15 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 6d41dd6f9f7a..1e28058b65a8 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -6840,6 +6840,13 @@ int stmmac_dvr_probe(struct device *device, > reset_control_reset(priv->plat->stmmac_rst); > } > > + if (priv->plat->stmmac_ahb_rst) { You don't need this conditional, stmmac_ahb_rst will be NULL if not specified and you can reset_control_deassert(NULL) without any problems. > + ret = reset_control_deassert(priv->plat->stmmac_ahb_rst); > + if (ret == -ENOTSUPP) > + dev_err(priv->device, > + "unable to bring out of ahb reset\n"); No need to wrap this line. > + } > + > /* Init MAC and get the capabilities */ > ret = stmmac_hw_init(priv); > if (ret) > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 97a1fedcc9ac..d8ae58bdbbe3 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -600,6 +600,13 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > goto error_hw_init; > } > > + plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared( > + &pdev->dev, "ahb"); > + if (IS_ERR(plat->stmmac_ahb_rst)) { > + ret = plat->stmmac_ahb_rst; You need a PTR_ERR() around the plat->stmmac_ahb_rst. Regards, Bjorn > + goto error_hw_init; > + } > + > return plat; > > error_hw_init: > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index e55a4807e3ea..9b6a64f3e3dc 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -239,6 +239,7 @@ struct plat_stmmacenet_data { > unsigned int mult_fact_100ns; > s32 ptp_max_adj; > struct reset_control *stmmac_rst; > + struct reset_control *stmmac_ahb_rst; > struct stmmac_axi *axi; > int has_gmac4; > bool has_sun8i; > -- > 2.26.3 >
On 06/06/2021 04:24, Bjorn Andersson wrote: > On Sat 05 Jun 12:35 CDT 2021, Matthew Hagan wrote: > >> We are currently assuming that GMAC_AHB_RESET will already be deasserted >> by the bootloader. However if this has not been done, probing of the GMAC >> will fail. To remedy this we must ensure GMAC_AHB_RESET has been deasserted >> prior to probing. >> > Sounds good, just some small style comments below. > >> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++ >> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 7 +++++++ >> include/linux/stmmac.h | 1 + >> 3 files changed, 15 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index 6d41dd6f9f7a..1e28058b65a8 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -6840,6 +6840,13 @@ int stmmac_dvr_probe(struct device *device, >> reset_control_reset(priv->plat->stmmac_rst); >> } >> >> + if (priv->plat->stmmac_ahb_rst) { > You don't need this conditional, stmmac_ahb_rst will be NULL if not > specified and you can reset_control_deassert(NULL) without any problems. > >> + ret = reset_control_deassert(priv->plat->stmmac_ahb_rst); >> + if (ret == -ENOTSUPP) >> + dev_err(priv->device, >> + "unable to bring out of ahb reset\n"); > No need to wrap this line. > >> + } >> + >> /* Init MAC and get the capabilities */ >> ret = stmmac_hw_init(priv); >> if (ret) >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 97a1fedcc9ac..d8ae58bdbbe3 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -600,6 +600,13 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) >> goto error_hw_init; >> } >> >> + plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared( >> + &pdev->dev, "ahb"); >> + if (IS_ERR(plat->stmmac_ahb_rst)) { >> + ret = plat->stmmac_ahb_rst; > You need a PTR_ERR() around the plat->stmmac_ahb_rst. > > Regards, > Bjorn > >> + goto error_hw_init; >> + } >> + >> return plat; >> >> error_hw_init: >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >> index e55a4807e3ea..9b6a64f3e3dc 100644 >> --- a/include/linux/stmmac.h >> +++ b/include/linux/stmmac.h >> @@ -239,6 +239,7 @@ struct plat_stmmacenet_data { >> unsigned int mult_fact_100ns; >> s32 ptp_max_adj; >> struct reset_control *stmmac_rst; >> + struct reset_control *stmmac_ahb_rst; >> struct stmmac_axi *axi; >> int has_gmac4; >> bool has_sun8i; >> -- >> 2.26.3 >> >> Thanks for the review. Will submit a v2 shortly. Matthew
On 06/06/2021 04:24, Bjorn Andersson wrote: >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 97a1fedcc9ac..d8ae58bdbbe3 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -600,6 +600,13 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) >> goto error_hw_init; >> } >> >> + plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared( >> + &pdev->dev, "ahb"); >> + if (IS_ERR(plat->stmmac_ahb_rst)) { >> + ret = plat->stmmac_ahb_rst; > You need a PTR_ERR() around the plat->stmmac_ahb_rst. This is giving a warning. Shouldn't v1 be kept as it is here? Please refer to "net: stmmac: platform: use optional clk/reset get APIs" [1] which modified error handling for plat->stmmac_rst. PTR_ERR() would then be called by the parent function on the returned value of ret. [1]: https://lore.kernel.org/netdev/20201112092606.5173aa6f@xhacker.debian/ Thanks, Matthew
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 6d41dd6f9f7a..1e28058b65a8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -6840,6 +6840,13 @@ int stmmac_dvr_probe(struct device *device, reset_control_reset(priv->plat->stmmac_rst); } + if (priv->plat->stmmac_ahb_rst) { + ret = reset_control_deassert(priv->plat->stmmac_ahb_rst); + if (ret == -ENOTSUPP) + dev_err(priv->device, + "unable to bring out of ahb reset\n"); + } + /* Init MAC and get the capabilities */ ret = stmmac_hw_init(priv); if (ret) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index 97a1fedcc9ac..d8ae58bdbbe3 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -600,6 +600,13 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) goto error_hw_init; } + plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared( + &pdev->dev, "ahb"); + if (IS_ERR(plat->stmmac_ahb_rst)) { + ret = plat->stmmac_ahb_rst; + goto error_hw_init; + } + return plat; error_hw_init: diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index e55a4807e3ea..9b6a64f3e3dc 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -239,6 +239,7 @@ struct plat_stmmacenet_data { unsigned int mult_fact_100ns; s32 ptp_max_adj; struct reset_control *stmmac_rst; + struct reset_control *stmmac_ahb_rst; struct stmmac_axi *axi; int has_gmac4; bool has_sun8i;
We are currently assuming that GMAC_AHB_RESET will already be deasserted by the bootloader. However if this has not been done, probing of the GMAC will fail. To remedy this we must ensure GMAC_AHB_RESET has been deasserted prior to probing. Signed-off-by: Matthew Hagan <mnhagan88@gmail.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++ drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 7 +++++++ include/linux/stmmac.h | 1 + 3 files changed, 15 insertions(+)