Hi,
Thanks, this version is almost perfect, unfortunately a second review
has found one small issue in it, see inline comment below.
On 07/24/2014 11:17 AM, Antoine Ténart wrote:
> 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 port_map mask is computed automatically when using
> this.
>
> Signed-off-by: Antoine Ténart <antoine.tenart@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/ata/ahci.h | 7 +-
> drivers/ata/libahci_platform.c | 190 ++++++++++++++++++++++++++++++++---------
> 2 files changed, 157 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index cb8d58926851..47de53284ad7 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -332,7 +332,12 @@ 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 */
> + /*
> + * If platform uses PHYs. There is a 1:1 relation between the port number and
> + * the PHY position in this array.
> + */
> + struct phy **phys;
> + unsigned nports; /* Number of ports */
> 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_platform.c b/drivers/ata/libahci_platform.c
> index db9b90d876dd..095df56432d9 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -39,6 +39,67 @@ 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 rc, i;
> +
> + for (i = 0; i < hpriv->nports; i++) {
> + if (!hpriv->phys[i])
> + continue;
> +
> + 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->nports; i++) {
> + if (!hpriv->phys[i])
> + continue;
> +
> + 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 +153,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 +175,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 +197,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 +237,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 +247,9 @@ 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, enabled_ports = 0, rc = -ENOMEM;
> + u32 mask_port_map = 0;
>
> if (!devres_open_group(dev, NULL, GFP_KERNEL))
> return ERR_PTR(-ENOMEM);
> @@ -246,27 +298,87 @@ 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 -ENOSYS:
> - /* No PHY support. Check if PHY is required. */
> - if (of_find_property(dev->of_node, "phys", NULL)) {
> - dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
> + hpriv->nports = of_get_child_count(dev->of_node);
> +
> + if (hpriv->nports) {
> + hpriv->phys = devm_kzalloc(dev,
> + hpriv->nports * sizeof(*hpriv->phys),
> + GFP_KERNEL);
> + if (!hpriv->phys) {
> + rc = -ENOMEM;
> + 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;
> }
> - case -ENODEV:
> - /* continue normally */
> - hpriv->phy = NULL;
> - break;
>
> - case -EPROBE_DEFER:
> - goto err_out;
> + if (port >= hpriv->nports) {
> + dev_warn(dev, "invalid port number %d\n", port);
> + continue;
> + }
>
> - default:
> - dev_err(dev, "couldn't get sata-phy\n");
> - goto err_out;
> + mask_port_map |= BIT(port);
> +
> + hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
> + if (IS_ERR(hpriv->phys[port])) {
> + rc = PTR_ERR(hpriv->phys[port]);
> + dev_err(dev,
> + "couldn't get PHY in node %s: %d\n",
> + child->name, rc);
> + goto err_out;
> + }
> +
> + enabled_ports++;
> + }
> + if (!enabled_ports) {
> + dev_warn(dev, "No port enabled\n");
> + return ERR_PTR(-ENODEV);
This should be:
rc = -ENODEV;
goto err_out;
Sorry for not catching that sooner.
Other then that the entire series looks good and is:
Acked-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Regards,
Hans
> + }
> +
> + if (!hpriv->mask_port_map)
> + hpriv->mask_port_map = mask_port_map;
> + } 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->nports = 1;
> + } else {
> + rc = PTR_ERR(phy);
> + switch (rc) {
> + case -ENOSYS:
> + /* No PHY support. Check if PHY is required. */
> + if (of_find_property(dev->of_node, "phys", NULL)) {
> + dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
> + goto err_out;
> + }
> + case -ENODEV:
> + /* continue normally */
> + hpriv->phys = NULL;
> + break;
> +
> + default:
> + goto err_out;
> +
> + }
> }
> }
>
> @@ -291,7 +403,7 @@ EXPORT_SYMBOL_GPL(ahci_platform_get_resources);
> * @host_flags: ahci host flags used in ahci_host_priv
> *
> * 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:
> @@ -395,7 +507,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:
> @@ -432,7 +544,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:
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
