* [PATCH 0/2] ata: ahci_ceva: fix xilinx GT PHY support
@ 2024-01-18 19:08 Radhey Shyam Pandey
2024-01-18 19:08 ` [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx " Radhey Shyam Pandey
2024-01-18 19:08 ` [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API " Radhey Shyam Pandey
0 siblings, 2 replies; 10+ messages in thread
From: Radhey Shyam Pandey @ 2024-01-18 19:08 UTC (permalink / raw)
To: dlemoal, cassel, richardcochran, piyush.mehta, axboe, michal.simek
Cc: linux-ide, linux-kernel, git, Radhey Shyam Pandey
This patchset add error handling for Xilinx GT PHY support.
It also fixes suspend warning 'Underflow of regulator enable count'.
xilinx-zcu102-20232:/home/petalinux# echo mem > /sys/power/state
[ 481.335785] PM: suspend entry (deep)
<snip>
[ 481.483682] sd 1:0:0:0: [sda] Synchronizing SCSI cache
[ 481.517440] macb ff0e0000.ethernet eth0: Link is Down
[ 481.523041] macb ff0e0000.ethernet: gem-ptp-timer ptp clock unregistered.
[ 481.530018] ata2.00: Entering standby power mode
[ 481.583873] regulator-dummy: Underflow of regulator enable count
[ 481.589876] regulator-dummy: Underflow of regulator enable count
[ 481.595883] regulator-dummy: Underflow of regulator enable count
Piyush Mehta (2):
ata: ahci_ceva: fix error handling for Xilinx GT PHY support
ata: ahci_ceva: add missing enable regulator API for Xilinx GT PHY
support
drivers/ata/ahci_ceva.c | 55 ++++++++++++++++++++++++++++++-----------
1 file changed, 41 insertions(+), 14 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
2024-01-18 19:08 [PATCH 0/2] ata: ahci_ceva: fix xilinx GT PHY support Radhey Shyam Pandey
@ 2024-01-18 19:08 ` Radhey Shyam Pandey
2024-01-22 7:39 ` Dan Carpenter
` (2 more replies)
2024-01-18 19:08 ` [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API " Radhey Shyam Pandey
1 sibling, 3 replies; 10+ messages in thread
From: Radhey Shyam Pandey @ 2024-01-18 19:08 UTC (permalink / raw)
To: dlemoal, cassel, richardcochran, piyush.mehta, axboe, michal.simek
Cc: linux-ide, linux-kernel, git, Piyush Mehta, Radhey Shyam Pandey
From: Piyush Mehta <piyush.mehta@amd.com>
Platform clock and phy error resources are not cleaned up in Xilinx GT PHY
error path. To fix introduce error label for ahci_platform_disable_clks and
phy_power_off/exit and call them in error path. No functional change.
Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
---
drivers/ata/ahci_ceva.c | 47 +++++++++++++++++++++++++++++------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
index 64f7f7d6ba84..bfc513f1d0b3 100644
--- a/drivers/ata/ahci_ceva.c
+++ b/drivers/ata/ahci_ceva.c
@@ -196,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
struct ahci_host_priv *hpriv;
struct ceva_ahci_priv *cevapriv;
enum dev_dma_attr attr;
- int rc;
+ int rc, i;
cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
if (!cevapriv)
@@ -219,8 +219,6 @@ static int ceva_ahci_probe(struct platform_device *pdev)
if (rc)
return rc;
} else {
- int i;
-
rc = ahci_platform_enable_clks(hpriv);
if (rc)
return rc;
@@ -229,8 +227,11 @@ static int ceva_ahci_probe(struct platform_device *pdev)
for (i = 0; i < hpriv->nports; i++) {
rc = phy_init(hpriv->phys[i]);
- if (rc)
- return rc;
+ if (rc) {
+ while (--i >= 0)
+ phy_exit(hpriv->phys[i]);
+ goto disable_clks;
+ }
}
/* De-assert the controller reset */
@@ -240,7 +241,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
rc = phy_power_on(hpriv->phys[i]);
if (rc) {
phy_exit(hpriv->phys[i]);
- return rc;
+ goto disable_phys;
}
}
}
@@ -252,52 +253,60 @@ static int ceva_ahci_probe(struct platform_device *pdev)
if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
(u8 *)&cevapriv->pp2c[0], 4) < 0) {
dev_warn(dev, "ceva,p0-cominit-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
(u8 *)&cevapriv->pp2c[1], 4) < 0) {
dev_warn(dev, "ceva,p1-cominit-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
/* Read OOB timing value for COMWAKE from device-tree*/
if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
(u8 *)&cevapriv->pp3c[0], 4) < 0) {
dev_warn(dev, "ceva,p0-comwake-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
(u8 *)&cevapriv->pp3c[1], 4) < 0) {
dev_warn(dev, "ceva,p1-comwake-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
/* Read phy BURST timing value from device-tree */
if (of_property_read_u8_array(np, "ceva,p0-burst-params",
(u8 *)&cevapriv->pp4c[0], 4) < 0) {
dev_warn(dev, "ceva,p0-burst-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
if (of_property_read_u8_array(np, "ceva,p1-burst-params",
(u8 *)&cevapriv->pp4c[1], 4) < 0) {
dev_warn(dev, "ceva,p1-burst-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
/* Read phy RETRY interval timing value from device-tree */
if (of_property_read_u16_array(np, "ceva,p0-retry-params",
(u16 *)&cevapriv->pp5c[0], 2) < 0) {
dev_warn(dev, "ceva,p0-retry-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
if (of_property_read_u16_array(np, "ceva,p1-retry-params",
(u16 *)&cevapriv->pp5c[1], 2) < 0) {
dev_warn(dev, "ceva,p1-retry-params property not defined\n");
- return -EINVAL;
+ rc = -EINVAL;
+ goto disable_phys;
}
/*
@@ -321,6 +330,16 @@ static int ceva_ahci_probe(struct platform_device *pdev)
disable_resources:
ahci_platform_disable_resources(hpriv);
+
+disable_phys:
+ while (--i >= 0) {
+ phy_power_off(hpriv->phys[i]);
+ phy_exit(hpriv->phys[i]);
+ }
+
+disable_clks:
+ ahci_platform_disable_clks(hpriv);
+
return rc;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API for Xilinx GT PHY support
2024-01-18 19:08 [PATCH 0/2] ata: ahci_ceva: fix xilinx GT PHY support Radhey Shyam Pandey
2024-01-18 19:08 ` [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx " Radhey Shyam Pandey
@ 2024-01-18 19:08 ` Radhey Shyam Pandey
2024-01-22 10:29 ` Damien Le Moal
2024-01-22 13:28 ` Niklas Cassel
1 sibling, 2 replies; 10+ messages in thread
From: Radhey Shyam Pandey @ 2024-01-18 19:08 UTC (permalink / raw)
To: dlemoal, cassel, richardcochran, piyush.mehta, axboe, michal.simek
Cc: linux-ide, linux-kernel, git, Piyush Mehta, Radhey Shyam Pandey
From: Piyush Mehta <piyush.mehta@amd.com>
The regulators API are disabled and enabled, during suspend and resume,
respectively. The following warning notice shows up on the initial suspend
because the enable regulators API is unaddressed in the probe:
regulator-dummy: Underflow of regulator enable count
Added the ahci_platform_enable_regulators API in probe to maintain the
regulator enabled and disabled ref count.
Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
drivers/ata/ahci_ceva.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
index bfc513f1d0b3..1c56f0cabb11 100644
--- a/drivers/ata/ahci_ceva.c
+++ b/drivers/ata/ahci_ceva.c
@@ -219,9 +219,14 @@ static int ceva_ahci_probe(struct platform_device *pdev)
if (rc)
return rc;
} else {
- rc = ahci_platform_enable_clks(hpriv);
+ rc = ahci_platform_enable_regulators(hpriv);
if (rc)
return rc;
+
+ rc = ahci_platform_enable_clks(hpriv);
+ if (rc)
+ goto disable_regulator;
+
/* Assert the controller reset */
reset_control_assert(cevapriv->rst);
@@ -340,6 +345,9 @@ static int ceva_ahci_probe(struct platform_device *pdev)
disable_clks:
ahci_platform_disable_clks(hpriv);
+disable_regulator:
+ ahci_platform_disable_regulators(hpriv);
+
return rc;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
2024-01-18 19:08 ` [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx " Radhey Shyam Pandey
@ 2024-01-22 7:39 ` Dan Carpenter
2024-01-22 10:28 ` Damien Le Moal
2024-01-22 13:02 ` Niklas Cassel
2 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2024-01-22 7:39 UTC (permalink / raw)
To: oe-kbuild, Radhey Shyam Pandey, dlemoal, cassel, richardcochran,
piyush.mehta, axboe, michal.simek
Cc: lkp, oe-kbuild-all, linux-ide, linux-kernel, git, Piyush Mehta,
Radhey Shyam Pandey
Hi Radhey,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Radhey-Shyam-Pandey/ata-ahci_ceva-fix-error-handling-for-Xilinx-GT-PHY-support/20240119-031129
base: linus/master
patch link: https://lore.kernel.org/r/1705604904-471889-2-git-send-email-radhey.shyam.pandey%40amd.com
patch subject: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
config: i386-randconfig-141-20240120 (https://download.01.org/0day-ci/archive/20240122/202401220603.dgjTZ08O-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202401220603.dgjTZ08O-lkp@intel.com/
smatch warnings:
drivers/ata/ahci_ceva.c:335 ceva_ahci_probe() error: uninitialized symbol 'i'.
vim +/i +335 drivers/ata/ahci_ceva.c
a73ed35052ca85 Suneel Garapati 2015-06-09 192 static int ceva_ahci_probe(struct platform_device *pdev)
a73ed35052ca85 Suneel Garapati 2015-06-09 193 {
a73ed35052ca85 Suneel Garapati 2015-06-09 194 struct device_node *np = pdev->dev.of_node;
a73ed35052ca85 Suneel Garapati 2015-06-09 195 struct device *dev = &pdev->dev;
a73ed35052ca85 Suneel Garapati 2015-06-09 196 struct ahci_host_priv *hpriv;
a73ed35052ca85 Suneel Garapati 2015-06-09 197 struct ceva_ahci_priv *cevapriv;
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 198 enum dev_dma_attr attr;
b1600f5880a13f Piyush Mehta 2024-01-19 199 int rc, i;
i needs to be initialized to zero here.
a73ed35052ca85 Suneel Garapati 2015-06-09 200
a73ed35052ca85 Suneel Garapati 2015-06-09 201 cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
a73ed35052ca85 Suneel Garapati 2015-06-09 202 if (!cevapriv)
a73ed35052ca85 Suneel Garapati 2015-06-09 203 return -ENOMEM;
a73ed35052ca85 Suneel Garapati 2015-06-09 204
a73ed35052ca85 Suneel Garapati 2015-06-09 205 cevapriv->ahci_pdev = pdev;
a73ed35052ca85 Suneel Garapati 2015-06-09 206
9a9d3abe24bb6b Piyush Mehta 2021-02-08 207 cevapriv->rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
9a9d3abe24bb6b Piyush Mehta 2021-02-08 208 NULL);
fa4b42b2a968dc Piyush Mehta 2021-03-05 209 if (IS_ERR(cevapriv->rst))
fa4b42b2a968dc Piyush Mehta 2021-03-05 210 dev_err_probe(&pdev->dev, PTR_ERR(cevapriv->rst),
fa4b42b2a968dc Piyush Mehta 2021-03-05 211 "failed to get reset\n");
9a9d3abe24bb6b Piyush Mehta 2021-02-08 212
16af2d65842d34 Kunihiko Hayashi 2018-08-22 213 hpriv = ahci_platform_get_resources(pdev, 0);
a73ed35052ca85 Suneel Garapati 2015-06-09 214 if (IS_ERR(hpriv))
a73ed35052ca85 Suneel Garapati 2015-06-09 215 return PTR_ERR(hpriv);
a73ed35052ca85 Suneel Garapati 2015-06-09 216
9a9d3abe24bb6b Piyush Mehta 2021-02-08 217 if (!cevapriv->rst) {
a73ed35052ca85 Suneel Garapati 2015-06-09 218 rc = ahci_platform_enable_resources(hpriv);
a73ed35052ca85 Suneel Garapati 2015-06-09 219 if (rc)
a73ed35052ca85 Suneel Garapati 2015-06-09 220 return rc;
i is uninitialized on this path.
9a9d3abe24bb6b Piyush Mehta 2021-02-08 221 } else {
9a9d3abe24bb6b Piyush Mehta 2021-02-08 222 rc = ahci_platform_enable_clks(hpriv);
9a9d3abe24bb6b Piyush Mehta 2021-02-08 223 if (rc)
9a9d3abe24bb6b Piyush Mehta 2021-02-08 224 return rc;
9a9d3abe24bb6b Piyush Mehta 2021-02-08 225 /* Assert the controller reset */
9a9d3abe24bb6b Piyush Mehta 2021-02-08 226 reset_control_assert(cevapriv->rst);
9a9d3abe24bb6b Piyush Mehta 2021-02-08 227
9a9d3abe24bb6b Piyush Mehta 2021-02-08 228 for (i = 0; i < hpriv->nports; i++) {
9a9d3abe24bb6b Piyush Mehta 2021-02-08 229 rc = phy_init(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta 2024-01-19 230 if (rc) {
b1600f5880a13f Piyush Mehta 2024-01-19 231 while (--i >= 0)
b1600f5880a13f Piyush Mehta 2024-01-19 232 phy_exit(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta 2024-01-19 233 goto disable_clks;
b1600f5880a13f Piyush Mehta 2024-01-19 234 }
9a9d3abe24bb6b Piyush Mehta 2021-02-08 235 }
9a9d3abe24bb6b Piyush Mehta 2021-02-08 236
9a9d3abe24bb6b Piyush Mehta 2021-02-08 237 /* De-assert the controller reset */
9a9d3abe24bb6b Piyush Mehta 2021-02-08 238 reset_control_deassert(cevapriv->rst);
9a9d3abe24bb6b Piyush Mehta 2021-02-08 239
9a9d3abe24bb6b Piyush Mehta 2021-02-08 240 for (i = 0; i < hpriv->nports; i++) {
9a9d3abe24bb6b Piyush Mehta 2021-02-08 241 rc = phy_power_on(hpriv->phys[i]);
9a9d3abe24bb6b Piyush Mehta 2021-02-08 242 if (rc) {
9a9d3abe24bb6b Piyush Mehta 2021-02-08 243 phy_exit(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta 2024-01-19 244 goto disable_phys;
9a9d3abe24bb6b Piyush Mehta 2021-02-08 245 }
9a9d3abe24bb6b Piyush Mehta 2021-02-08 246 }
9a9d3abe24bb6b Piyush Mehta 2021-02-08 247 }
a73ed35052ca85 Suneel Garapati 2015-06-09 248
a73ed35052ca85 Suneel Garapati 2015-06-09 249 if (of_property_read_bool(np, "ceva,broken-gen2"))
a73ed35052ca85 Suneel Garapati 2015-06-09 250 cevapriv->flags = CEVA_FLAG_BROKEN_GEN2;
a73ed35052ca85 Suneel Garapati 2015-06-09 251
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 252 /* Read OOB timing value for COMINIT from device-tree */
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 253 if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 254 (u8 *)&cevapriv->pp2c[0], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 255 dev_warn(dev, "ceva,p0-cominit-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 256 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 257 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 258 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 259
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 260 if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 261 (u8 *)&cevapriv->pp2c[1], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 262 dev_warn(dev, "ceva,p1-cominit-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 263 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 264 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 265 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 266
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 267 /* Read OOB timing value for COMWAKE from device-tree*/
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 268 if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 269 (u8 *)&cevapriv->pp3c[0], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 270 dev_warn(dev, "ceva,p0-comwake-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 271 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 272 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 273 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 274
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 275 if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 276 (u8 *)&cevapriv->pp3c[1], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 277 dev_warn(dev, "ceva,p1-comwake-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 278 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 279 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 280 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 281
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 282 /* Read phy BURST timing value from device-tree */
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 283 if (of_property_read_u8_array(np, "ceva,p0-burst-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 284 (u8 *)&cevapriv->pp4c[0], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 285 dev_warn(dev, "ceva,p0-burst-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 286 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 287 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 288 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 289
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 290 if (of_property_read_u8_array(np, "ceva,p1-burst-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 291 (u8 *)&cevapriv->pp4c[1], 4) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 292 dev_warn(dev, "ceva,p1-burst-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 293 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 294 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 295 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 296
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 297 /* Read phy RETRY interval timing value from device-tree */
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 298 if (of_property_read_u16_array(np, "ceva,p0-retry-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 299 (u16 *)&cevapriv->pp5c[0], 2) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 300 dev_warn(dev, "ceva,p0-retry-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 301 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 302 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 303 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 304
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 305 if (of_property_read_u16_array(np, "ceva,p1-retry-params",
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 306 (u16 *)&cevapriv->pp5c[1], 2) < 0) {
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 307 dev_warn(dev, "ceva,p1-retry-params property not defined\n");
b1600f5880a13f Piyush Mehta 2024-01-19 308 rc = -EINVAL;
b1600f5880a13f Piyush Mehta 2024-01-19 309 goto disable_phys;
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 310 }
fe8365bbf8ac58 Anurag Kumar Vulisha 2017-08-21 311
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 312 /*
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 313 * Check if CCI is enabled for SATA. The DEV_DMA_COHERENT is returned
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 314 * if CCI is enabled, so check for DEV_DMA_COHERENT.
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 315 */
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 316 attr = device_get_dma_attr(dev);
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 317 cevapriv->is_cci_enabled = (attr == DEV_DMA_COHERENT);
3bc867de85b5bf Anurag Kumar Vulisha 2017-08-21 318
a73ed35052ca85 Suneel Garapati 2015-06-09 319 hpriv->plat_data = cevapriv;
a73ed35052ca85 Suneel Garapati 2015-06-09 320
a73ed35052ca85 Suneel Garapati 2015-06-09 321 /* CEVA specific initialization */
a73ed35052ca85 Suneel Garapati 2015-06-09 322 ahci_ceva_setup(hpriv);
a73ed35052ca85 Suneel Garapati 2015-06-09 323
a73ed35052ca85 Suneel Garapati 2015-06-09 324 rc = ahci_platform_init_host(pdev, hpriv, &ahci_ceva_port_info,
a73ed35052ca85 Suneel Garapati 2015-06-09 325 &ahci_platform_sht);
a73ed35052ca85 Suneel Garapati 2015-06-09 326 if (rc)
a73ed35052ca85 Suneel Garapati 2015-06-09 327 goto disable_resources;
a73ed35052ca85 Suneel Garapati 2015-06-09 328
a73ed35052ca85 Suneel Garapati 2015-06-09 329 return 0;
a73ed35052ca85 Suneel Garapati 2015-06-09 330
a73ed35052ca85 Suneel Garapati 2015-06-09 331 disable_resources:
a73ed35052ca85 Suneel Garapati 2015-06-09 332 ahci_platform_disable_resources(hpriv);
b1600f5880a13f Piyush Mehta 2024-01-19 333
b1600f5880a13f Piyush Mehta 2024-01-19 334 disable_phys:
b1600f5880a13f Piyush Mehta 2024-01-19 @335 while (--i >= 0) {
^^^
b1600f5880a13f Piyush Mehta 2024-01-19 336 phy_power_off(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta 2024-01-19 337 phy_exit(hpriv->phys[i]);
b1600f5880a13f Piyush Mehta 2024-01-19 338 }
b1600f5880a13f Piyush Mehta 2024-01-19 339
b1600f5880a13f Piyush Mehta 2024-01-19 340 disable_clks:
b1600f5880a13f Piyush Mehta 2024-01-19 341 ahci_platform_disable_clks(hpriv);
b1600f5880a13f Piyush Mehta 2024-01-19 342
a73ed35052ca85 Suneel Garapati 2015-06-09 343 return rc;
a73ed35052ca85 Suneel Garapati 2015-06-09 344 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
2024-01-18 19:08 ` [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx " Radhey Shyam Pandey
2024-01-22 7:39 ` Dan Carpenter
@ 2024-01-22 10:28 ` Damien Le Moal
2024-01-22 13:02 ` Niklas Cassel
2 siblings, 0 replies; 10+ messages in thread
From: Damien Le Moal @ 2024-01-22 10:28 UTC (permalink / raw)
To: Radhey Shyam Pandey, cassel, richardcochran, piyush.mehta, axboe,
michal.simek
Cc: linux-ide, linux-kernel, git, Piyush Mehta
On 1/19/24 04:08, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.com>
>
> Platform clock and phy error resources are not cleaned up in Xilinx GT PHY
> error path. To fix introduce error label for ahci_platform_disable_clks and
> phy_power_off/exit and call them in error path. No functional change.
>
> Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> ---
Your patch format is strange... There is one too many "---" line here.
Other than that, looks OK to me.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API for Xilinx GT PHY support
2024-01-18 19:08 ` [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API " Radhey Shyam Pandey
@ 2024-01-22 10:29 ` Damien Le Moal
2024-01-22 13:28 ` Niklas Cassel
1 sibling, 0 replies; 10+ messages in thread
From: Damien Le Moal @ 2024-01-22 10:29 UTC (permalink / raw)
To: Radhey Shyam Pandey, cassel, richardcochran, piyush.mehta, axboe,
michal.simek
Cc: linux-ide, linux-kernel, git, Piyush Mehta
On 1/19/24 04:08, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.com>
>
> The regulators API are disabled and enabled, during suspend and resume,
> respectively. The following warning notice shows up on the initial suspend
> because the enable regulators API is unaddressed in the probe:
>
> regulator-dummy: Underflow of regulator enable count
>
> Added the ahci_platform_enable_regulators API in probe to maintain the
> regulator enabled and disabled ref count.
>
> Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Looks OK to me.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
2024-01-18 19:08 ` [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx " Radhey Shyam Pandey
2024-01-22 7:39 ` Dan Carpenter
2024-01-22 10:28 ` Damien Le Moal
@ 2024-01-22 13:02 ` Niklas Cassel
2024-02-07 18:28 ` Pandey, Radhey Shyam
2 siblings, 1 reply; 10+ messages in thread
From: Niklas Cassel @ 2024-01-22 13:02 UTC (permalink / raw)
To: Radhey Shyam Pandey
Cc: dlemoal, richardcochran, piyush.mehta, axboe, michal.simek,
linux-ide, linux-kernel, git, Piyush Mehta
Hello Radhey,
On Fri, Jan 19, 2024 at 12:38:23AM +0530, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.com>
>
> Platform clock and phy error resources are not cleaned up in Xilinx GT PHY
> error path. To fix introduce error label for ahci_platform_disable_clks and
> phy_power_off/exit and call them in error path. No functional change.
>
> Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> ---
> drivers/ata/ahci_ceva.c | 47 +++++++++++++++++++++++++++++------------
> 1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
> index 64f7f7d6ba84..bfc513f1d0b3 100644
> --- a/drivers/ata/ahci_ceva.c
> +++ b/drivers/ata/ahci_ceva.c
> @@ -196,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
> struct ahci_host_priv *hpriv;
> struct ceva_ahci_priv *cevapriv;
> enum dev_dma_attr attr;
> - int rc;
> + int rc, i;
>
> cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
> if (!cevapriv)
> @@ -219,8 +219,6 @@ static int ceva_ahci_probe(struct platform_device *pdev)
> if (rc)
> return rc;
> } else {
> - int i;
> -
> rc = ahci_platform_enable_clks(hpriv);
> if (rc)
> return rc;
> @@ -229,8 +227,11 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>
> for (i = 0; i < hpriv->nports; i++) {
> rc = phy_init(hpriv->phys[i]);
> - if (rc)
> - return rc;
> + if (rc) {
> + while (--i >= 0)
> + phy_exit(hpriv->phys[i]);
It is ugly to have a loop both here and at the end of the function.
Why don't you just goto disable_phys here?
Just like how it is done in ahci_platform_enable_phys():
https://github.com/torvalds/linux/blob/v6.7/drivers/ata/libahci_platform.c#L52-L54
> + goto disable_clks;
> + }
> }
>
> /* De-assert the controller reset */
> @@ -240,7 +241,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
> rc = phy_power_on(hpriv->phys[i]);
> if (rc) {
> phy_exit(hpriv->phys[i]);
> - return rc;
> + goto disable_phys;
> }
> }
> }
> @@ -252,52 +253,60 @@ static int ceva_ahci_probe(struct platform_device *pdev)
> if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
> (u8 *)&cevapriv->pp2c[0], 4) < 0) {
> dev_warn(dev, "ceva,p0-cominit-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
> (u8 *)&cevapriv->pp2c[1], 4) < 0) {
> dev_warn(dev, "ceva,p1-cominit-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> /* Read OOB timing value for COMWAKE from device-tree*/
> if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
> (u8 *)&cevapriv->pp3c[0], 4) < 0) {
> dev_warn(dev, "ceva,p0-comwake-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
> (u8 *)&cevapriv->pp3c[1], 4) < 0) {
> dev_warn(dev, "ceva,p1-comwake-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> /* Read phy BURST timing value from device-tree */
> if (of_property_read_u8_array(np, "ceva,p0-burst-params",
> (u8 *)&cevapriv->pp4c[0], 4) < 0) {
> dev_warn(dev, "ceva,p0-burst-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> if (of_property_read_u8_array(np, "ceva,p1-burst-params",
> (u8 *)&cevapriv->pp4c[1], 4) < 0) {
> dev_warn(dev, "ceva,p1-burst-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> /* Read phy RETRY interval timing value from device-tree */
> if (of_property_read_u16_array(np, "ceva,p0-retry-params",
> (u16 *)&cevapriv->pp5c[0], 2) < 0) {
> dev_warn(dev, "ceva,p0-retry-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> if (of_property_read_u16_array(np, "ceva,p1-retry-params",
> (u16 *)&cevapriv->pp5c[1], 2) < 0) {
> dev_warn(dev, "ceva,p1-retry-params property not defined\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto disable_phys;
> }
>
> /*
> @@ -321,6 +330,16 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>
> disable_resources:
> ahci_platform_disable_resources(hpriv);
Looking at ahci_platform_disable_resources(),
it calls ahci_platform_disable_phys(), which calls
phy_power_off() and phy_exit().
This means that if you jump to disable_resources,
you will call phy_power_off() and phy_exit() twice.
Looking at the phy code, it does not handle these functions being called
twice.
You already call ahci_platform_get_resources(), so why don't you just set
AHCI_PLATFORM_GET_RESETS, that way you should be able to remove a bunch of
duplicated code from this driver.
One major difference seems to be that ahci_platform_enable_resources() does
not assert reset before deasserting it.
Can't we just add a reset_control_assert() + some small usleep
(e.g. usleep_range(1000, 1500)) before the reset_control_deassert()?
Have you tried doing that?
Kind regards,
Niklas
> +
> +disable_phys:
> + while (--i >= 0) {
> + phy_power_off(hpriv->phys[i]);
> + phy_exit(hpriv->phys[i]);
> + }
> +
> +disable_clks:
> + ahci_platform_disable_clks(hpriv);
> +
> return rc;
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API for Xilinx GT PHY support
2024-01-18 19:08 ` [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API " Radhey Shyam Pandey
2024-01-22 10:29 ` Damien Le Moal
@ 2024-01-22 13:28 ` Niklas Cassel
1 sibling, 0 replies; 10+ messages in thread
From: Niklas Cassel @ 2024-01-22 13:28 UTC (permalink / raw)
To: Radhey Shyam Pandey
Cc: dlemoal, richardcochran, piyush.mehta, axboe, michal.simek,
linux-ide, linux-kernel, git, Piyush Mehta
Hello Radhey,
On Fri, Jan 19, 2024 at 12:38:24AM +0530, Radhey Shyam Pandey wrote:
> From: Piyush Mehta <piyush.mehta@amd.com>
>
> The regulators API are disabled and enabled, during suspend and resume,
> respectively. The following warning notice shows up on the initial suspend
> because the enable regulators API is unaddressed in the probe:
Please be a bit more specific in your commit message.
e.g. during system suspend, ahci_platform_suspend() calls
ahci_platform_disable_resources() which calls
ahci_platform_disable_regulators() which calls
regulator_disable() for all regulators found in the controller.
>
> regulator-dummy: Underflow of regulator enable count
>
> Added the ahci_platform_enable_regulators API in probe to maintain the
> regulator enabled and disabled ref count.
s/Added/Add/
"Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour."
see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
> Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support xilinx GT phy")
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> drivers/ata/ahci_ceva.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
> index bfc513f1d0b3..1c56f0cabb11 100644
> --- a/drivers/ata/ahci_ceva.c
> +++ b/drivers/ata/ahci_ceva.c
> @@ -219,9 +219,14 @@ static int ceva_ahci_probe(struct platform_device *pdev)
> if (rc)
> return rc;
> } else {
> - rc = ahci_platform_enable_clks(hpriv);
> + rc = ahci_platform_enable_regulators(hpriv);
> if (rc)
> return rc;
> +
> + rc = ahci_platform_enable_clks(hpriv);
> + if (rc)
> + goto disable_regulator;
> +
Like I wrote in patch 1/2, I would prefer if you could somehow get
ahci_platform_enable_resources() to work for your platform, so that you
don't need to copy paste all of ahci_platform_enable_resources() to
your driver.
If it does not work to simply add a reset_control_assert() + usleep(),
considering that this function is essentially a copy paste of
ahci_platform_enable_resources(), I would still prefer the addition of
a new flag, and keep the extra logic needed in libahci_platform.c, so that
the code is kept in the same place, rather than to copy paste the whole
function to your driver.
Kind regards,
Niklas
> /* Assert the controller reset */
> reset_control_assert(cevapriv->rst);
>
> @@ -340,6 +345,9 @@ static int ceva_ahci_probe(struct platform_device *pdev)
> disable_clks:
> ahci_platform_disable_clks(hpriv);
>
> +disable_regulator:
> + ahci_platform_disable_regulators(hpriv);
> +
> return rc;
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
2024-01-22 13:02 ` Niklas Cassel
@ 2024-02-07 18:28 ` Pandey, Radhey Shyam
2024-02-07 20:42 ` Niklas Cassel
0 siblings, 1 reply; 10+ messages in thread
From: Pandey, Radhey Shyam @ 2024-02-07 18:28 UTC (permalink / raw)
To: Niklas Cassel
Cc: dlemoal, richardcochran, piyush.mehta, axboe, Simek, Michal,
linux-ide, linux-kernel, git (AMD-Xilinx),
Mehta, Piyush
> -----Original Message-----
> From: Niklas Cassel <cassel@kernel.org>
> Sent: Monday, January 22, 2024 6:33 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: dlemoal@kernel.org; richardcochran@gmail.com;
> piyush.mehta@xilinx.com; axboe@kernel.dk; Simek, Michal
> <michal.simek@amd.com>; linux-ide@vger.kernel.org; linux-
> kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>; Mehta, Piyush
> <piyush.mehta@amd.com>
> Subject: Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY
> support
>
> Hello Radhey,
>
> On Fri, Jan 19, 2024 at 12:38:23AM +0530, Radhey Shyam Pandey wrote:
> > From: Piyush Mehta <piyush.mehta@amd.com>
> >
> > Platform clock and phy error resources are not cleaned up in Xilinx GT
> > PHY error path. To fix introduce error label for
> > ahci_platform_disable_clks and phy_power_off/exit and call them in error
> path. No functional change.
> >
> > Fixes: 9a9d3abe24bb ("ata: ahci: ceva: Update the driver to support
> > xilinx GT phy")
> > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > ---
> > ---
> > drivers/ata/ahci_ceva.c | 47
> > +++++++++++++++++++++++++++++------------
> > 1 file changed, 33 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c index
> > 64f7f7d6ba84..bfc513f1d0b3 100644
> > --- a/drivers/ata/ahci_ceva.c
> > +++ b/drivers/ata/ahci_ceva.c
> > @@ -196,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device
> *pdev)
> > struct ahci_host_priv *hpriv;
> > struct ceva_ahci_priv *cevapriv;
> > enum dev_dma_attr attr;
> > - int rc;
> > + int rc, i;
> >
> > cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
> > if (!cevapriv)
> > @@ -219,8 +219,6 @@ static int ceva_ahci_probe(struct platform_device
> *pdev)
> > if (rc)
> > return rc;
> > } else {
> > - int i;
> > -
> > rc = ahci_platform_enable_clks(hpriv);
> > if (rc)
> > return rc;
> > @@ -229,8 +227,11 @@ static int ceva_ahci_probe(struct platform_device
> > *pdev)
> >
> > for (i = 0; i < hpriv->nports; i++) {
> > rc = phy_init(hpriv->phys[i]);
> > - if (rc)
> > - return rc;
> > + if (rc) {
> > + while (--i >= 0)
> > + phy_exit(hpriv->phys[i]);
>
> It is ugly to have a loop both here and at the end of the function.
> Why don't you just goto disable_phys here?
>
> Just like how it is done in ahci_platform_enable_phys():
> https://github.com/torvalds/linux/blob/v6.7/drivers/ata/libahci_platform.c#
> L52-L54
>
>
> > + goto disable_clks;
> > + }
> > }
> >
> > /* De-assert the controller reset */ @@ -240,7 +241,7 @@
> static int
> > ceva_ahci_probe(struct platform_device *pdev)
> > rc = phy_power_on(hpriv->phys[i]);
> > if (rc) {
> > phy_exit(hpriv->phys[i]);
> > - return rc;
> > + goto disable_phys;
> > }
> > }
> > }
> > @@ -252,52 +253,60 @@ static int ceva_ahci_probe(struct
> platform_device *pdev)
> > if (of_property_read_u8_array(np, "ceva,p0-cominit-params",
> > (u8 *)&cevapriv->pp2c[0], 4) < 0) {
> > dev_warn(dev, "ceva,p0-cominit-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u8_array(np, "ceva,p1-cominit-params",
> > (u8 *)&cevapriv->pp2c[1], 4) < 0) {
> > dev_warn(dev, "ceva,p1-cominit-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /* Read OOB timing value for COMWAKE from device-tree*/
> > if (of_property_read_u8_array(np, "ceva,p0-comwake-params",
> > (u8 *)&cevapriv->pp3c[0], 4) < 0) {
> > dev_warn(dev, "ceva,p0-comwake-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u8_array(np, "ceva,p1-comwake-params",
> > (u8 *)&cevapriv->pp3c[1], 4) < 0) {
> > dev_warn(dev, "ceva,p1-comwake-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /* Read phy BURST timing value from device-tree */
> > if (of_property_read_u8_array(np, "ceva,p0-burst-params",
> > (u8 *)&cevapriv->pp4c[0], 4) < 0) {
> > dev_warn(dev, "ceva,p0-burst-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u8_array(np, "ceva,p1-burst-params",
> > (u8 *)&cevapriv->pp4c[1], 4) < 0) {
> > dev_warn(dev, "ceva,p1-burst-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /* Read phy RETRY interval timing value from device-tree */
> > if (of_property_read_u16_array(np, "ceva,p0-retry-params",
> > (u16 *)&cevapriv->pp5c[0], 2) < 0) {
> > dev_warn(dev, "ceva,p0-retry-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > if (of_property_read_u16_array(np, "ceva,p1-retry-params",
> > (u16 *)&cevapriv->pp5c[1], 2) < 0) {
> > dev_warn(dev, "ceva,p1-retry-params property not
> defined\n");
> > - return -EINVAL;
> > + rc = -EINVAL;
> > + goto disable_phys;
> > }
> >
> > /*
> > @@ -321,6 +330,16 @@ static int ceva_ahci_probe(struct platform_device
> > *pdev)
> >
> > disable_resources:
> > ahci_platform_disable_resources(hpriv);
>
> Looking at ahci_platform_disable_resources(),
> it calls ahci_platform_disable_phys(), which calls
> phy_power_off() and phy_exit().
>
> This means that if you jump to disable_resources, you will call
> phy_power_off() and phy_exit() twice.
> Looking at the phy code, it does not handle these functions being called
> twice.
>
>
> You already call ahci_platform_get_resources(), so why don't you just set
> AHCI_PLATFORM_GET_RESETS, that way you should be able to remove a
> bunch of duplicated code from this driver.
>
> One major difference seems to be that ahci_platform_enable_resources()
> does not assert reset before deasserting it.
> Can't we just add a reset_control_assert() + some small usleep (e.g.
> usleep_range(1000, 1500)) before the reset_control_deassert()?
> Have you tried doing that?
As I understand AHCI-platform library supports shared reset control
lines and AHCI SATA platform driver request exclusive reset lines.
So we need to move reset request out of ahci platform library and
make it driver specific as done in commit "9a9d3abe24bb
("ata: ahci: ceva: Update the driver to support xilinx GT phy")
Furthermore SATA IP programming mentioned in
Zynq UltraScale+ Device TRM mentions -
Assert SATA reset.
Set PS-GTR configuration (Part of phy_init)
Bring SATA by de-asserting the reset.
Now looking at ahci_platform_enable_resources(), it
enables all ahci_platform managed resources in the
following order
1) Regulator
2) Clocks (through ahci_platform_enable_clks)
3) Resets
4) Phys
Here reset is de-asserted before phy_init() which
is different from SATA Controller Programming Flow.
I assume because of above programming sequence
differences we earlier didn't use generic
ahci_platform_enable_resources() and instead used
a custom implementation derived from it in ceva
AHCI SATA platform driver.
Coming to reusing ahci_platform_enable_resources(),
if we have to do we have skip step-3. and let ceva
ahci driver request reset and then call
reset_control_assert()/deassert.
However for single controller IP programming sequence
deviation I am less inclined to make modification in
generic enable_resources() API . But if you strongly
feel we should go ahead and make changes to the generic
enable_resources API, please suggest your thoughts
and then we can get started.
Thanks,
Radhey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx GT PHY support
2024-02-07 18:28 ` Pandey, Radhey Shyam
@ 2024-02-07 20:42 ` Niklas Cassel
0 siblings, 0 replies; 10+ messages in thread
From: Niklas Cassel @ 2024-02-07 20:42 UTC (permalink / raw)
To: Pandey, Radhey Shyam
Cc: dlemoal, richardcochran, piyush.mehta, axboe, Simek, Michal,
linux-ide, linux-kernel, git (AMD-Xilinx),
Mehta, Piyush
Hello Radhey,
On Wed, Feb 07, 2024 at 06:28:25PM +0000, Pandey, Radhey Shyam wrote:
(snip)
> However for single controller IP programming sequence
> deviation I am less inclined to make modification in
> generic enable_resources() API . But if you strongly
> feel we should go ahead and make changes to the generic
> enable_resources API, please suggest your thoughts
> and then we can get started.
My first suggestion was to try to add a reset_control_assert() + msleep(1)
before the reset_control_deassert() in ahci_platform_enable_resources().
This should be safe for all platforms as in order to trigger a reset,
you actually need to pulse it. It is possible that your platform has
reset deasserted from boot, and in that case, since the generic version
currently only does a deassert, it is possible that no reset was done
at all on your platform.
If my first suggestion did not work, then my second suggestion was to
add a new flag which your platform driver sets
(e.g. HOLD_RESET_DURING_PHY_INIT), and implement that in a new function
in libahci_platform.c. The generic version could check for this flag at
the start of the function, and return the result from your new function
in libahci_platform.c instead.
However, since you have a TRM, let's ignore these two proposals and instead
re-spin your existing series, addressing the other outstanding issues:
1) The new kernel test robot build warnings introduced by your patch.
- The other review comments that are still applicable:
2) Your patch format is strange... There is one too many "---" line here.
3) Use goto disable_phys so you don't need to loop twice.
4) If you jump to disable_resources, you will call phy_power_off()
and phy_exit() twice, which the PHY lib does not handle.
5) Please be a bit more specific in your commit message.
6) Describe your changes in imperative mood.
7) A new comment that I see when looking at your driver just now,
you should simply remove:
"""
if (!cevapriv->rst) {
rc = ahci_platform_enable_resources(hpriv);
if (rc)
return rc;
} else {
"""
Either you want to use ahci_platform_enable_resources(),
or your own way, but you don't get to do both.
You call devm_reset_control_get_optional_exclusive(), which will
return NULL for platforms that do not supply a reset.
Both reset_control_assert() and reset_control_deassert() are no-ops
if you send in a NULL pointer. The are designed this way to work
with devm_reset_control_get_optional_exclusive() which will return
NULL for platforms that do not provide a reset, such that drivers,
such as yours, should not need
if () {
} else {
depending on if get_reset() returned non-NULL or not.
Just always call reset_control_assert()/reset_control_deassert(),
even for the !cevapriv->rst case, as they will be no-ops anyway.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-02-07 20:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-18 19:08 [PATCH 0/2] ata: ahci_ceva: fix xilinx GT PHY support Radhey Shyam Pandey
2024-01-18 19:08 ` [PATCH 1/2] ata: ahci_ceva: fix error handling for Xilinx " Radhey Shyam Pandey
2024-01-22 7:39 ` Dan Carpenter
2024-01-22 10:28 ` Damien Le Moal
2024-01-22 13:02 ` Niklas Cassel
2024-02-07 18:28 ` Pandey, Radhey Shyam
2024-02-07 20:42 ` Niklas Cassel
2024-01-18 19:08 ` [PATCH 2/2] ata: ahci_ceva: add missing enable regulator API " Radhey Shyam Pandey
2024-01-22 10:29 ` Damien Le Moal
2024-01-22 13:28 ` Niklas Cassel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.