Message-ID: <20220825055333.kvdv3ddys2dcu3dg@mobilestation>
Date:   Thu, 25 Aug 2022 08:53:33 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>
Cc:     Hans de Goede <hdegoede@...hat.com>, Jens Axboe <axboe@...nel.dk>,
        Hannes Reinecke <hare@...e.de>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v7 07/23] ata: libahci_platform: Convert to using devm
 bulk clocks API

On Wed, Aug 24, 2022 at 04:19:54PM -0700, Damien Le Moal wrote:
> On 2022/08/22 11:37, Serge Semin wrote:
> > In order to simplify the clock-related code there is a way to convert the
> > current fixed clocks array into using the common bulk clocks kernel API
> > with dynamic set of the clock handlers and device-managed clock-resource
> > tracking. It's a bit tricky due to the complication coming from the
> > requirement to support the platforms (da850, spear13xx) with the
> > non-OF-based clock source, but still doable.
> > 
> > Before this modification there are two methods have been used to get the
> > clocks connected to an AHCI device: clk_get() - to get the very first
> > clock in the list and of_clk_get() - to get the rest of them. Basically
> > the platforms with non-OF-based clocks definition could specify only a
> > single reference clock source. The platforms with OF-hw clocks have been
> > luckier and could setup up to AHCI_MAX_CLKS clocks. Such semantic can be
> > retained with using devm_clk_bulk_get_all() to retrieve the clocks defined
> > via the DT firmware and devm_clk_get_optional() otherwise. In both cases
> > using the device-managed version of the methods will cause the automatic
> > resources deallocation on the AHCI device removal event. The only
> > complicated part in the suggested approach is the explicit allocation and
> > initialization of the clk_bulk_data structure instance for the non-OF
> > reference clocks. It's required in order to use the Bulk Clocks API for
> > the both denoted cases of the clocks definition.
> > 
> > Note aside with the clock-related code reduction and natural
> > simplification, there are several bonuses the suggested modification
> > provides. First of all the limitation of having no greater than
> > AHCI_MAX_CLKS clocks is now removed, since the devm_clk_bulk_get_all()
> > method will allocate as many reference clocks data descriptors as there
> > are clocks specified for the device. Secondly the clock names are
> > auto-detected. So the LLDD (glue) drivers can make sure that the required
> > clocks are specified just by checking the clock IDs in the clk_bulk_data
> > array.  Thirdly using the handy Bulk Clocks kernel API improves the
> > clocks-handling code readability. And the last but not least this
> > modification implements a true optional clocks support to the
> > ahci_platform_get_resources() method. Indeed the previous clocks getting
> > procedure just stopped getting the clocks on any errors (aside from
> > non-critical -EPROBE_DEFER) in a way so the callee wasn't even informed
> > about abnormal loop termination. The new implementation lacks of such
> > problem. The ahci_platform_get_resources() will return an error code if
> > the corresponding clocks getting method ends execution abnormally.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > Reviewed-by: Hannes Reinecke <hare@...e.de>
> 

> The build bot complains about this patch. Please see the build report.

What an unfortunate patch.) Thanks for noting.

To sum up alas I can't use __clk_get_name() to get the default clock
name because the method is undefined for the platforms with no clock
support. But we still need some name of the clock connection ID. Seeing
in case of a single clock case, the clock connection ID isn't that
needed I'll just use a default ID like "ahci".

-Sergey

> 
> > 
> > ---
> > 
> > Changelog v2:
> > - Convert to checking the error-case first in the devm_clk_bulk_get_all()
> >   method invocation. (@Damien)
> > - Fix some grammar mistakes in the comments.
> > 
> > Changelog v7:
> > - Fix Davinci DA850 and Omap2 DM816 AHCI LLDD to be using the new bulk
> >   clocks interface.
> > ---
> >  drivers/ata/ahci.h             |  4 +-
> >  drivers/ata/ahci_da850.c       | 47 ++++++++-----------
> >  drivers/ata/ahci_dm816.c       |  4 +-
> >  drivers/ata/libahci_platform.c | 84 ++++++++++++++++------------------
> >  4 files changed, 62 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> > index ad11a4c52fbe..c3770a19781b 100644
> > --- a/drivers/ata/ahci.h
> > +++ b/drivers/ata/ahci.h
> > @@ -38,7 +38,6 @@
> >  
> >  enum {
> >  	AHCI_MAX_PORTS		= 32,
> > -	AHCI_MAX_CLKS		= 5,
> >  	AHCI_MAX_SG		= 168, /* hardware max is 64K */
> >  	AHCI_DMA_BOUNDARY	= 0xffffffff,
> >  	AHCI_MAX_CMDS		= 32,
> > @@ -339,7 +338,8 @@ struct ahci_host_priv {
> >  	u32			em_msg_type;	/* EM message type */
> >  	u32			remapped_nvme;	/* NVMe remapped device count */
> >  	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
> > -	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
> > +	unsigned int		n_clks;
> > +	struct clk_bulk_data	*clks;		/* Optional */
> >  	struct reset_control	*rsts;		/* Optional */
> >  	struct regulator	**target_pwrs;	/* Optional */
> >  	struct regulator	*ahci_regulator;/* Optional */
> > diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
> > index 052c28e250aa..dc8a019b8340 100644
> > --- a/drivers/ata/ahci_da850.c
> > +++ b/drivers/ata/ahci_da850.c
> > @@ -163,7 +163,6 @@ static int ahci_da850_probe(struct platform_device *pdev)
> >  	struct ahci_host_priv *hpriv;
> >  	void __iomem *pwrdn_reg;
> >  	struct resource *res;
> > -	struct clk *clk;
> >  	u32 mpy;
> >  	int rc;
> >  
> > @@ -172,36 +171,28 @@ static int ahci_da850_probe(struct platform_device *pdev)
> >  		return PTR_ERR(hpriv);
> >  
> >  	/*
> > -	 * Internally ahci_platform_get_resources() calls clk_get(dev, NULL)
> > -	 * when trying to obtain the functional clock. This SATA controller
> > -	 * uses two clocks for which we specify two connection ids. If we don't
> > -	 * have the functional clock at this point - call clk_get() again with
> > -	 * con_id = "fck".
> > +	 * Internally ahci_platform_get_resources() calls the bulk clocks
> > +	 * get method or falls back to using a single clk_get_optional().
> > +	 * This AHCI SATA controller uses two clocks: functional clock
> > +	 * with "fck" connection id and external reference clock with
> > +	 * "refclk" id. If we haven't got all of them re-try the clocks
> > +	 * getting procedure with the explicitly specified ids.
> >  	 */
> > -	if (!hpriv->clks[0]) {
> > -		clk = clk_get(dev, "fck");
> > -		if (IS_ERR(clk))
> > -			return PTR_ERR(clk);
> > -
> > -		hpriv->clks[0] = clk;
> > -	}
> > -
> > -	/*
> > -	 * The second clock used by ahci-da850 is the external REFCLK. If we
> > -	 * didn't get it from ahci_platform_get_resources(), let's try to
> > -	 * specify the con_id in clk_get().
> > -	 */
> > -	if (!hpriv->clks[1]) {
> > -		clk = clk_get(dev, "refclk");
> > -		if (IS_ERR(clk)) {
> > -			dev_err(dev, "unable to obtain the reference clock");
> > -			return -ENODEV;
> > -		}
> > -
> > -		hpriv->clks[1] = clk;
> > +	if (hpriv->n_clks < 2) {
> > +		hpriv->clks = devm_kcalloc(dev, 2, sizeof(*hpriv->clks), GFP_KERNEL);
> > +		if (!hpriv->clks)
> > +			return -ENOMEM;
> > +
> > +		hpriv->clks[0].id = "fck";
> > +		hpriv->clks[1].id = "refclk";
> > +		hpriv->n_clks = 2;
> > +
> > +		rc = devm_clk_bulk_get(dev, hpriv->n_clks, hpriv->clks);
> > +		if (rc)
> > +			return rc;
> >  	}
> >  
> > -	mpy = ahci_da850_calculate_mpy(clk_get_rate(hpriv->clks[1]));
> > +	mpy = ahci_da850_calculate_mpy(clk_get_rate(hpriv->clks[1].clk));
> >  	if (mpy == 0) {
> >  		dev_err(dev, "invalid REFCLK multiplier value: 0x%x", mpy);
> >  		return -EINVAL;
> > diff --git a/drivers/ata/ahci_dm816.c b/drivers/ata/ahci_dm816.c
> > index 8a92112dcd59..d26efcd20f64 100644
> > --- a/drivers/ata/ahci_dm816.c
> > +++ b/drivers/ata/ahci_dm816.c
> > @@ -69,12 +69,12 @@ static int ahci_dm816_phy_init(struct ahci_host_priv *hpriv, struct device *dev)
> >  	 * keep-alive clock and the external reference clock. We need the
> >  	 * rate of the latter to calculate the correct value of MPY bits.
> >  	 */
> > -	if (!hpriv->clks[1]) {
> > +	if (hpriv->n_clks < 2) {
> >  		dev_err(dev, "reference clock not supplied\n");
> >  		return -EINVAL;
> >  	}
> >  
> > -	refclk_rate = clk_get_rate(hpriv->clks[1]);
> > +	refclk_rate = clk_get_rate(hpriv->clks[1].clk);
> >  	if ((refclk_rate % 100) != 0) {
> >  		dev_err(dev, "reference clock rate must be divisible by 100\n");
> >  		return -EINVAL;
> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> > index 1e9e825d6cc5..814804582d1d 100644
> > --- a/drivers/ata/libahci_platform.c
> > +++ b/drivers/ata/libahci_platform.c
> > @@ -8,6 +8,7 @@
> >   *   Anton Vorontsov <avorontsov@...mvista.com>
> >   */
> >  
> > +#include <linux/clk-provider.h>
> >  #include <linux/clk.h>
> >  #include <linux/kernel.h>
> >  #include <linux/gfp.h>
> > @@ -97,28 +98,14 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_phys);
> >   * ahci_platform_enable_clks - Enable platform clocks
> >   * @hpriv: host private area to store config values
> >   *
> > - * This function enables all the clks found in hpriv->clks, starting at
> > - * index 0. If any clk fails to enable it disables all the clks already
> > - * enabled in reverse order, and then returns an error.
> > + * This function enables all the clks found for the AHCI device.
> >   *
> >   * RETURNS:
> >   * 0 on success otherwise a negative error code
> >   */
> >  int ahci_platform_enable_clks(struct ahci_host_priv *hpriv)
> >  {
> > -	int c, rc;
> > -
> > -	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) {
> > -		rc = clk_prepare_enable(hpriv->clks[c]);
> > -		if (rc)
> > -			goto disable_unprepare_clk;
> > -	}
> > -	return 0;
> > -
> > -disable_unprepare_clk:
> > -	while (--c >= 0)
> > -		clk_disable_unprepare(hpriv->clks[c]);
> > -	return rc;
> > +	return clk_bulk_prepare_enable(hpriv->n_clks, hpriv->clks);
> >  }
> >  EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);
> >  
> > @@ -126,16 +113,13 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_clks);
> >   * ahci_platform_disable_clks - Disable platform clocks
> >   * @hpriv: host private area to store config values
> >   *
> > - * This function disables all the clks found in hpriv->clks, in reverse
> > - * order of ahci_platform_enable_clks (starting at the end of the array).
> > + * This function disables all the clocks enabled before
> > + * (bulk-clocks-disable function is supposed to do that in reverse
> > + * from the enabling procedure order).
> >   */
> >  void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
> >  {
> > -	int c;
> > -
> > -	for (c = AHCI_MAX_CLKS - 1; c >= 0; c--)
> > -		if (hpriv->clks[c])
> > -			clk_disable_unprepare(hpriv->clks[c]);
> > +	clk_bulk_disable_unprepare(hpriv->n_clks, hpriv->clks);
> >  }
> >  EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
> >  
> > @@ -292,8 +276,6 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
> >  		pm_runtime_disable(dev);
> >  	}
> >  
> > -	for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++)
> > -		clk_put(hpriv->clks[c]);
> >  	/*
> >  	 * The regulators are tied to child node device and not to the
> >  	 * SATA device itself. So we can't use devm for automatically
> > @@ -374,8 +356,8 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> >   * 1) mmio registers (IORESOURCE_MEM 0, mandatory)
> >   * 2) regulator for controlling the targets power (optional)
> >   *    regulator for controlling the AHCI controller (optional)
> > - * 3) 0 - AHCI_MAX_CLKS clocks, as specified in the devs devicetree node,
> > - *    or for non devicetree enabled platforms a single clock
> > + * 3) all clocks specified in the devicetree node, or a single
> > + *    clock for non-OF platforms (optional)
> >   * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
> >   * 5) phys (optional)
> >   *
> > @@ -385,11 +367,10 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
> >  struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >  						   unsigned int flags)
> >  {
> > +	int child_nodes, rc = -ENOMEM, enabled_ports = 0;
> >  	struct device *dev = &pdev->dev;
> >  	struct ahci_host_priv *hpriv;
> > -	struct clk *clk;
> >  	struct device_node *child;
> > -	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
> >  	u32 mask_port_map = 0;
> >  
> >  	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> > @@ -415,25 +396,38 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >  		goto err_out;
> >  	}
> >  
> > -	for (i = 0; i < AHCI_MAX_CLKS; i++) {
> > +	/*
> > +	 * Bulk clocks getting procedure can fail to find any clock due to
> > +	 * running on a non-OF platform or due to the clocks being defined in
> > +	 * bypass of the DT firmware (like da850, spear13xx). In that case we
> > +	 * fallback to getting a single clock source right from the dev clocks
> > +	 * list.
> > +	 */
> > +	rc = devm_clk_bulk_get_all(dev, &hpriv->clks);
> > +	if (rc < 0)
> > +		goto err_out;
> > +
> > +	if (rc > 0) {
> > +		/* Got clocks in bulk */
> > +		hpriv->n_clks = rc;
> > +	} else {
> >  		/*
> > -		 * For now we must use clk_get(dev, NULL) for the first clock,
> > -		 * because some platforms (da850, spear13xx) are not yet
> > -		 * converted to use devicetree for clocks.  For new platforms
> > -		 * this is equivalent to of_clk_get(dev->of_node, 0).
> > +		 * No clock bulk found: fallback to manually getting
> > +		 * the optional clock.
> >  		 */
> > -		if (i == 0)
> > -			clk = clk_get(dev, NULL);
> > -		else
> > -			clk = of_clk_get(dev->of_node, i);
> > -
> > -		if (IS_ERR(clk)) {
> > -			rc = PTR_ERR(clk);
> > -			if (rc == -EPROBE_DEFER)
> > -				goto err_out;
> > -			break;
> > +		hpriv->clks = devm_kzalloc(dev, sizeof(*hpriv->clks), GFP_KERNEL);
> > +		if (!hpriv->clks) {
> > +			rc = -ENOMEM;
> > +			goto err_out;
> > +		}
> > +		hpriv->clks->clk = devm_clk_get_optional(dev, NULL);
> > +		if (IS_ERR(hpriv->clks->clk)) {
> > +			rc = PTR_ERR(hpriv->clks->clk);
> > +			goto err_out;
> > +		} else if (hpriv->clks->clk) {
> > +			hpriv->clks->id = __clk_get_name(hpriv->clks->clk);
> > +			hpriv->n_clks = 1;
> >  		}
> > -		hpriv->clks[i] = clk;
> >  	}
> >  
> >  	hpriv->ahci_regulator = devm_regulator_get(dev, "ahci");
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research

Powered by blists - more mailing lists