| Message ID | 1404728173-20263-4-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
(Cc'ing Hans.) On Mon, Jul 07, 2014 at 12:16:09PM +0200, Antoine Ténart wrote: > @@ -482,6 +482,13 @@ void ahci_save_initial_config(struct device *dev, > port_map &= mask_port_map; > } > > + /* > + * If port_map was filled automatically when finding port sub-nodes, > + * make sure we get the right set here. > + */ > + if (hpriv->port_map) > + port_map &= hpriv->port_map; > + So, hpriv->port is both input and output? This is messy and can lead to confusing failures and there now are multiple ways to modify port_map. If carrying this information through ahci_host_priv is necessary, let's remove the direct params and introduce new input fields to the struct. > /** > + * ahci_platform_enable_phys - Enable PHYs > + * @hpriv: host private area to store config values > + * > + * This function enables all the PHYs found in hpriv->phys, if any. > + * If a PHY fails to be enabled, it disables all the PHYs already > + * enabled in reverse order and returns an error. > + * > + * RETURNS: > + * 0 on success otherwise a negative error code > + */ > +int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) > +{ > + int i, rc = 0; > + > + for (i = 0; i < hpriv->nphys; i++) { > + rc = phy_init(hpriv->phys[i]); > + if (rc) > + goto disable_phys; > + > + rc = phy_power_on(hpriv->phys[i]); > + if (rc) { > + phy_exit(hpriv->phys[i]); > + goto disable_phys; > + } > + } > + > + return 0; > + > +disable_phys: > + while (--i >= 0) { > + phy_power_off(hpriv->phys[i]); > + phy_exit(hpriv->phys[i]); > + } > + return rc; > +} > +EXPORT_SYMBOL_GPL(ahci_platform_enable_phys); Do we need to make hpriv->phys[] dynamically allocated? We already have hpriv->clks[AHCI_MAX_CLKS] and it's unlikely that we're gonna need more than several phys per host. Let's go with a simpler fixed array. Thanks.
One more thing. On Mon, Jul 07, 2014 at 12:16:09PM +0200, Antoine Ténart wrote: > This introduce a new way of defining SATA ports in the device tree, with > one port per sub-node. This as the advantage of allowing a per port > configuration. Because some ports may be accessible but disabled in the > device tree, the default port_map is computed automatically when using > this. Any chance the old one phy config can be moved over to this scheme so that we don't have to carry two separate mechanisms? Thanks.
Hello, On Tue, Jul 08, 2014 at 09:40:00AM -0400, Tejun Heo wrote: > (Cc'ing Hans.) > > On Mon, Jul 07, 2014 at 12:16:09PM +0200, Antoine Ténart wrote: > > @@ -482,6 +482,13 @@ void ahci_save_initial_config(struct device *dev, > > port_map &= mask_port_map; > > } > > > > + /* > > + * If port_map was filled automatically when finding port sub-nodes, > > + * make sure we get the right set here. > > + */ > > + if (hpriv->port_map) > > + port_map &= hpriv->port_map; > > + > > So, hpriv->port is both input and output? This is messy and can lead > to confusing failures and there now are multiple ways to modify > port_map. If carrying this information through ahci_host_priv is > necessary, let's remove the direct params and introduce new input > fields to the struct. We just use hpriv->port_map to check port_map is valid and describes available ports there. hpriv->port_map is filed by the generic ahci_platform_get_resources() function when using the new bindings and not by the drivers. port_map is the input from the drivers. > > > /** > > + * ahci_platform_enable_phys - Enable PHYs > > + * @hpriv: host private area to store config values > > + * > > + * This function enables all the PHYs found in hpriv->phys, if any. > > + * If a PHY fails to be enabled, it disables all the PHYs already > > + * enabled in reverse order and returns an error. > > + * > > + * RETURNS: > > + * 0 on success otherwise a negative error code > > + */ > > +int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) > > +{ > > + int i, rc = 0; > > + > > + for (i = 0; i < hpriv->nphys; i++) { > > + rc = phy_init(hpriv->phys[i]); > > + if (rc) > > + goto disable_phys; > > + > > + rc = phy_power_on(hpriv->phys[i]); > > + if (rc) { > > + phy_exit(hpriv->phys[i]); > > + goto disable_phys; > > + } > > + } > > + > > + return 0; > > + > > +disable_phys: > > + while (--i >= 0) { > > + phy_power_off(hpriv->phys[i]); > > + phy_exit(hpriv->phys[i]); > > + } > > + return rc; > > +} > > +EXPORT_SYMBOL_GPL(ahci_platform_enable_phys); > > Do we need to make hpriv->phys[] dynamically allocated? We already > have hpriv->clks[AHCI_MAX_CLKS] and it's unlikely that we're gonna > need more than several phys per host. Let's go with a simpler fixed > array. > Well, a had a review a week ago about in the PHY driver saying I should avoid using fixed sized arrays... And it was in a driver were we know the maximum number of PHY available. I think in this case were the number of PHYs depends on the h/w, we should use a dynamically allocated array. Antoine
On Tue, Jul 08, 2014 at 09:42:03AM -0400, Tejun Heo wrote: > One more thing. > > On Mon, Jul 07, 2014 at 12:16:09PM +0200, Antoine Ténart wrote: > > This introduce a new way of defining SATA ports in the device tree, with > > one port per sub-node. This as the advantage of allowing a per port > > configuration. Because some ports may be accessible but disabled in the > > device tree, the default port_map is computed automatically when using > > this. > > Any chance the old one phy config can be moved over to this scheme so > that we don't have to carry two separate mechanisms? That would break the DT ABI. Antoine
Hello, On Tue, Jul 08, 2014 at 07:03:53PM +0200, Antoine Ténart wrote: > > So, hpriv->port is both input and output? This is messy and can lead > > to confusing failures and there now are multiple ways to modify > > port_map. If carrying this information through ahci_host_priv is > > necessary, let's remove the direct params and introduce new input > > fields to the struct. > > We just use hpriv->port_map to check port_map is valid and describes > available ports there. > > hpriv->port_map is filed by the generic ahci_platform_get_resources() > function when using the new bindings and not by the drivers. port_map is > the input from the drivers. So, yeah, it's being used both as input and output and we also have the arguments which affect port_map, right? It does seem confusing. > Well, a had a review a week ago about in the PHY driver saying I should > avoid using fixed sized arrays... And it was in a driver were we know > the maximum number of PHY available. > > I think in this case were the number of PHYs depends on the h/w, we should > use a dynamically allocated array. Well, so does clk. Let's say clk is more restricted and phy can be one or more per port and thus needs to be dynamic. If so, shouldn't we at least have some correlation between phys and ports? It bothers me that now libahci is carrying random number of resources that it has no idea how to associate with the ports it manages. What if later we want to involve phy driver in power managing unoccupied ports? Thanks.
On Tue, Jul 08, 2014 at 01:18:17PM -0400, Tejun Heo wrote: > Hello, > > On Tue, Jul 08, 2014 at 07:03:53PM +0200, Antoine Ténart wrote: > > > So, hpriv->port is both input and output? This is messy and can lead > > > to confusing failures and there now are multiple ways to modify > > > port_map. If carrying this information through ahci_host_priv is > > > necessary, let's remove the direct params and introduce new input > > > fields to the struct. > > > > We just use hpriv->port_map to check port_map is valid and describes > > available ports there. > > > > hpriv->port_map is filed by the generic ahci_platform_get_resources() > > function when using the new bindings and not by the drivers. port_map is > > the input from the drivers. > > So, yeah, it's being used both as input and output and we also have > the arguments which affect port_map, right? It does seem confusing. I do see priv->port_map as being automatically set and then restricted if needed by the port_map input here. I don't see how that's confusing. The only modification is we restrict the port_map parameter to the set of available ports. The port_map argument affected priv->port_map before this patch. > > > Well, a had a review a week ago about in the PHY driver saying I should > > avoid using fixed sized arrays... And it was in a driver were we know > > the maximum number of PHY available. > > > > I think in this case were the number of PHYs depends on the h/w, we should > > use a dynamically allocated array. > > Well, so does clk. Let's say clk is more restricted and phy can be > one or more per port and thus needs to be dynamic. If so, shouldn't > we at least have some correlation between phys and ports? It bothers > me that now libahci is carrying random number of resources that it has > no idea how to associate with the ports it manages. What if later we > want to involve phy driver in power managing unoccupied ports? I see. This is a first (working) attempt to have a one node per port. I agree that would be nice to have a correlation between ports and PHYs. This can definitively be added when needed without changing the dt bindings as only the internal representation changes. This would also require to get all phys from the port nodes, which is again internal stuff. Don't you think we can go by steps, and have a following up series for this when needed (like in a power managing series for unoccupied ports)? Antoine
Hey, On Tue, Jul 08, 2014 at 07:49:00PM +0200, Antoine Ténart wrote: > > So, yeah, it's being used both as input and output and we also have > > the arguments which affect port_map, right? It does seem confusing. > > I do see priv->port_map as being automatically set and then restricted > if needed by the port_map input here. I don't see how that's confusing. > The only modification is we restrict the port_map parameter to the set > of available ports. The port_map argument affected priv->port_map before > this patch. It is confusing. If you wanna pass around available ports in hpriv, please add a separate field and replace the arguments to save_initial_config(). > > Well, so does clk. Let's say clk is more restricted and phy can be > > one or more per port and thus needs to be dynamic. If so, shouldn't > > we at least have some correlation between phys and ports? It bothers > > me that now libahci is carrying random number of resources that it has > > no idea how to associate with the ports it manages. What if later we > > want to involve phy driver in power managing unoccupied ports? > > I see. This is a first (working) attempt to have a one node per port. I > agree that would be nice to have a correlation between ports and PHYs. > This can definitively be added when needed without changing the dt > bindings as only the internal representation changes. This would also > require to get all phys from the port nodes, which is again internal > stuff. > > Don't you think we can go by steps, and have a following up series for > this when needed (like in a power managing series for unoccupied ports)? I don't know. It isn't exactly difficult to make it per-port, is it? We already have ahci_port_priv and wouldn't the code actually be simpler that way? Thanks.
Hi,
On 07/08/2014 03:40 PM, Tejun Heo wrote:
> (Cc'ing Hans.)
Thanks for adding me to the loop.
I've been reading the entire thread sofar, and here are my 2 cents:
1) I think overall this is a good idea, and I like the suggested dt representation
2) I agree with Tejun that it would be better to tie the phy to the port, and thus to
add the struct phy * pointer to struct ahci_port_priv.
One question 2 brings with it is how to deal with older device-tree files which
have the phy directly in the ahci controller node. This seems to only be the
case for the TI dra7 and omap5 platforms. Probably those have only 1 sata port
(this needs to be verified), if that is the case we can simply hard code
tying the phy to port 0 in this case, and we should probably also update their
dts files in the kernel tree to use the new structure (while keeping the
ahci driver compatible with the old structure for now).
Regards,
Hans
Hello, On Wed, Jul 09, 2014 at 10:23:31AM +0200, Antoine Ténart wrote: > > It is confusing. If you wanna pass around available ports in hpriv, > > please add a separate field and replace the arguments to > > save_initial_config(). > > I don't get it. Which argument should I replace in > save_initial_config()? The change is we compute hpriv->port_map. > I don't see which arguments we can add or replace. The @force_port_map and @mask_port_map of ahci_save_initial_config(). We end up with three params from two places modifying port_map and one of those is in/out parameter, so ummm, no. If you wanna add port masking to @hpriv, please do it by moving @force_port_map and @mask_port_map into @hpriv instead. Sure, the proposed change is small but the end result is messy. > I had a quick look on this, and it does not seems to be that simple. The > ahci_port_priv is stored inside the ata_port struct and not accessible > (as of now) from the ahci_host_priv one. The ahci_port_priv is > initialized at the end of ahci_platform_init_host(), far after we need > it. This requires quite a lot of changes. Or is there another way? Yeah, it'd probably need separating out port resource handling, so that the order is get_resources, host_alloc, get_port_resources and then init and activate. Hans, what do you think? > To be honest, we are now at v9 and it's been quite a long time since v1. > I'd really like it to be merged in 3.17. As I see it, this patch keeps > the same logic as what was in place before, only with more PHYs. > > Don't take me wrong, I really think this is a good idea to have a > per-port PHY information. But this is a refactoring not clearly related > to this series as the logic is not changed. This definitively can be the > subject of a dedicated series, especially if I got it right and the > required modifications are not that obvious. Heh, I get that but, at the same time, this is the point where you're most motivated to actually work on it. :) Let's see how much work it's gonna be. Thanks.
Hi, On 07/09/2014 03:59 PM, Tejun Heo wrote: > Hello, > > On Wed, Jul 09, 2014 at 10:23:31AM +0200, Antoine Ténart wrote: >>> It is confusing. If you wanna pass around available ports in hpriv, >>> please add a separate field and replace the arguments to >>> save_initial_config(). >> >> I don't get it. Which argument should I replace in >> save_initial_config()? The change is we compute hpriv->port_map. >> I don't see which arguments we can add or replace. > > The @force_port_map and @mask_port_map of ahci_save_initial_config(). > We end up with three params from two places modifying port_map and one > of those is in/out parameter, so ummm, no. If you wanna add port > masking to @hpriv, please do it by moving @force_port_map and > @mask_port_map into @hpriv instead. > > Sure, the proposed change is small but the end result is messy. > >> I had a quick look on this, and it does not seems to be that simple. The >> ahci_port_priv is stored inside the ata_port struct and not accessible >> (as of now) from the ahci_host_priv one. The ahci_port_priv is >> initialized at the end of ahci_platform_init_host(), far after we need >> it. This requires quite a lot of changes. Or is there another way? > > Yeah, it'd probably need separating out port resource handling, so > that the order is get_resources, host_alloc, get_port_resources and > then init and activate. Hans, what do you think? The order (and function names) you're suggesting here sound good to me. Regards, Hans
Hi, On 09/07/2014 at 09:59:10 -0400, Tejun Heo wrote : > Heh, I get that but, at the same time, this is the point where you're > most motivated to actually work on it. :) > Or, starting that kind of review after 9 revisions, especially so close to the end of the development cycle is discouraging enough and will probably be used as an example of why we still have so many evil vendor trees.
On Wed, Jul 09, 2014 at 05:24:46PM +0200, Alexandre Belloni wrote: > Hi, > > On 09/07/2014 at 09:59:10 -0400, Tejun Heo wrote : > > Heh, I get that but, at the same time, this is the point where you're > > most motivated to actually work on it. :) > > > > Or, starting that kind of review after 9 revisions, especially so close > to the end of the development cycle is discouraging enough and will > probably be used as an example of why we still have so many evil vendor > trees. Yeap, that's my bad. I should have paid more attention. Sorry about that. We still have quite a few weeks to go tho, so I don't think it's too unreasonable to at least look into it. We're gonna have to do it anyway. Thanks.
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index 05882e4445a6..e5bdbf827ca8 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -330,7 +330,8 @@ struct ahci_host_priv { bool got_runtime_pm; /* Did we do pm_runtime_get? */ struct clk *clks[AHCI_MAX_CLKS]; /* Optional */ struct regulator *target_pwr; /* Optional */ - struct phy *phy; /* If platform uses phy */ + struct phy **phys; /* If platform uses phys */ + unsigned nphys; /* Number of phys */ void *plat_data; /* Other platform data */ /* * Optional ahci_start_engine override, if not set this gets set to the diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 40ea583d3610..0bdee8330dcf 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -482,6 +482,13 @@ void ahci_save_initial_config(struct device *dev, port_map &= mask_port_map; } + /* + * If port_map was filled automatically when finding port sub-nodes, + * make sure we get the right set here. + */ + if (hpriv->port_map) + port_map &= hpriv->port_map; + /* cross check port_map and cap.n_ports */ if (port_map) { int map_ports = 0; diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 3a5b4ed25a4f..cd2bc50ba913 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -39,6 +39,61 @@ static struct scsi_host_template ahci_platform_sht = { }; /** + * ahci_platform_enable_phys - Enable PHYs + * @hpriv: host private area to store config values + * + * This function enables all the PHYs found in hpriv->phys, if any. + * If a PHY fails to be enabled, it disables all the PHYs already + * enabled in reverse order and returns an error. + * + * RETURNS: + * 0 on success otherwise a negative error code + */ +int ahci_platform_enable_phys(struct ahci_host_priv *hpriv) +{ + int i, rc = 0; + + for (i = 0; i < hpriv->nphys; i++) { + rc = phy_init(hpriv->phys[i]); + if (rc) + goto disable_phys; + + rc = phy_power_on(hpriv->phys[i]); + if (rc) { + phy_exit(hpriv->phys[i]); + goto disable_phys; + } + } + + return 0; + +disable_phys: + while (--i >= 0) { + phy_power_off(hpriv->phys[i]); + phy_exit(hpriv->phys[i]); + } + return rc; +} +EXPORT_SYMBOL_GPL(ahci_platform_enable_phys); + +/** + * ahci_platform_disable_phys - Enable PHYs + * @hpriv: host private area to store config values + * + * This function disables all PHYs found in hpriv->phys. + */ +void ahci_platform_disable_phys(struct ahci_host_priv *hpriv) +{ + int i; + + for (i = 0; i < hpriv->nphys; i++) { + phy_power_off(hpriv->phys[i]); + phy_exit(hpriv->phys[i]); + } +} +EXPORT_SYMBOL_GPL(ahci_platform_disable_phys); + +/** * ahci_platform_enable_clks - Enable platform clocks * @hpriv: host private area to store config values * @@ -92,7 +147,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_clks); * following order: * 1) Regulator * 2) Clocks (through ahci_platform_enable_clks) - * 3) Phy + * 3) Phys * * If resource enabling fails at any point the previous enabled resources * are disabled in reverse order. @@ -114,17 +169,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv) if (rc) goto disable_regulator; - if (hpriv->phy) { - rc = phy_init(hpriv->phy); - if (rc) - goto disable_clks; - - rc = phy_power_on(hpriv->phy); - if (rc) { - phy_exit(hpriv->phy); - goto disable_clks; - } - } + rc = ahci_platform_enable_phys(hpriv); + if (rc) + goto disable_clks; return 0; @@ -144,16 +191,13 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_resources); * * This function disables all ahci_platform managed resources in the * following order: - * 1) Phy + * 1) Phys * 2) Clocks (through ahci_platform_disable_clks) * 3) Regulator */ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv) { - if (hpriv->phy) { - phy_power_off(hpriv->phy); - phy_exit(hpriv->phy); - } + ahci_platform_disable_phys(hpriv); ahci_platform_disable_clks(hpriv); @@ -187,7 +231,7 @@ static void ahci_platform_put_resources(struct device *dev, void *res) * 2) regulator for controlling the targets power (optional) * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node, * or for non devicetree enabled platforms a single clock - * 4) phy (optional) + * 4) phys (optional) * * RETURNS: * The allocated ahci_host_priv on success, otherwise an ERR_PTR value @@ -197,7 +241,8 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) struct device *dev = &pdev->dev; struct ahci_host_priv *hpriv; struct clk *clk; - int i, rc = -ENOMEM; + struct device_node *child; + int i, nports, rc = -ENOMEM; if (!devres_open_group(dev, NULL, GFP_KERNEL)) return ERR_PTR(-ENOMEM); @@ -246,22 +291,77 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) hpriv->clks[i] = clk; } - hpriv->phy = devm_phy_get(dev, "sata-phy"); - if (IS_ERR(hpriv->phy)) { - rc = PTR_ERR(hpriv->phy); - switch (rc) { - case -ENODEV: - case -ENOSYS: - /* continue normally */ - hpriv->phy = NULL; - break; + nports = of_get_child_count(dev->of_node); - case -EPROBE_DEFER: + if (nports) { + hpriv->phys = devm_kzalloc(dev, nports * sizeof(*hpriv->phys), + GFP_KERNEL); + if (!hpriv->phys) { + rc = -ENOMEM; goto err_out; + } - default: - dev_err(dev, "couldn't get sata-phy\n"); - goto err_out; + for_each_child_of_node(dev->of_node, child) { + u32 port; + + if (!of_device_is_available(child)) + continue; + + if (of_property_read_u32(child, "reg", &port)) { + rc = -EINVAL; + goto err_out; + } + + hpriv->port_map |= BIT(port); + + hpriv->phys[hpriv->nphys] = devm_of_phy_get(dev, child, + NULL); + if (IS_ERR(hpriv->phys[hpriv->nphys])) { + rc = PTR_ERR(hpriv->phys[hpriv->nphys]); + dev_err(dev, + "couldn't get PHY in node %s: %d\n", + child->name, rc); + goto err_out; + } + + hpriv->nphys++; + } + + if (!hpriv->nphys) { + dev_warn(dev, "No port enabled\n"); + return ERR_PTR(-ENODEV); + } + } else { + /* + * If no sub-node was found, keep this for device tree + * compatibility + */ + struct phy *phy = devm_phy_get(dev, "sata-phy"); + if (!IS_ERR(phy)) { + hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys), + GFP_KERNEL); + if (!hpriv->phys) { + rc = -ENOMEM; + goto err_out; + } + + hpriv->phys[0] = phy; + hpriv->nphys = 1; + } else { + rc = PTR_ERR(phy); + switch (rc) { + case -ENODEV: + case -ENOSYS: + /* continue normally */ + break; + + case -EPROBE_DEFER: + goto err_out; + + default: + dev_err(dev, "couldn't get sata-phy\n"); + goto err_out; + } } } @@ -288,7 +388,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_get_resources); * @mask_port_map: param passed to ahci_save_initial_config * * This function does all the usual steps needed to bring up an - * ahci-platform host, note any necessary resources (ie clks, phy, etc.) + * ahci-platform host, note any necessary resources (ie clks, phys, etc.) * must be initialized / enabled before calling this. * * RETURNS: @@ -394,7 +494,7 @@ static void ahci_host_stop(struct ata_host *host) * @dev: device pointer for the host * * This function does all the usual steps needed to suspend an - * ahci-platform host, note any necessary resources (ie clks, phy, etc.) + * ahci-platform host, note any necessary resources (ie clks, phys, etc.) * must be disabled after calling this. * * RETURNS: @@ -431,7 +531,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend_host); * @dev: device pointer for the host * * This function does all the usual steps needed to resume an ahci-platform - * host, note any necessary resources (ie clks, phy, etc.) must be + * host, note any necessary resources (ie clks, phys, etc.) must be * initialized / enabled before calling this. * * RETURNS:
The current implementation of the libahci does not allow to use multiple PHYs. This patch adds the support of multiple PHYs by the libahci while keeping the old bindings valid for device tree compatibility. This introduce a new way of defining SATA ports in the device tree, with one port per sub-node. This as the advantage of allowing a per port configuration. Because some ports may be accessible but disabled in the device tree, the default port_map is computed automatically when using this. Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> --- drivers/ata/ahci.h | 3 +- drivers/ata/libahci.c | 7 ++ drivers/ata/libahci_platform.c | 170 ++++++++++++++++++++++++++++++++--------- 3 files changed, 144 insertions(+), 36 deletions(-)