linux-kernel - Re: [PATCH 07/10] sata: ahci_da850: add support for the da850,clk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <357b3929-cf5c-43ce-753e-03ccd7583f18@lechnology.com>
Date:   Fri, 13 Jan 2017 13:29:36 -0600
From:   David Lechner <david@...hnology.com>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Kevin Hilman <khilman@...nel.org>,
        Sekhar Nori <nsekhar@...com>,
        Patrick Titiano <ptitiano@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Tejun Heo <tj@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>
Cc:     linux-ide@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 07/10] sata: ahci_da850: add support for the
 da850,clk_multiplier DT property

On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
> Currently the clock multiplier is hardcoded in the driver for
> the da850-evm board. Make it configurable over DT, but keep the
> previous value as default in case the property is missing.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> ---
>  drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
> index bb9eb4c..cd04caf 100644
> --- a/drivers/ata/ahci_da850.c
> +++ b/drivers/ata/ahci_da850.c
> @@ -28,17 +28,70 @@
>  #define SATA_PHY_TXSWING(x)	((x) << 19)
>  #define SATA_PHY_ENPLL(x)	((x) << 31)
>
> +struct da850_sata_mpy_mapping {
> +	unsigned int multiplier;
> +	unsigned int regval;
> +};
> +
> +static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
> +	{
> +		.multiplier	= 5,
> +		.regval		= 0x01,
> +	},
> +	{
> +		.multiplier	= 6,
> +		.regval		= 0x02,
> +	},
> +	{
> +		.multiplier	= 8,
> +		.regval		= 0x04,
> +	},
> +	{
> +		.multiplier	= 10,
> +		.regval		= 0x05,
> +	},
> +	{
> +		.multiplier	= 12,
> +		.regval		= 0x06,
> +	},
> +	/* TODO Add 12.5 multiplier. */

Looks like you should be using an enum here for the multiplier field.

> +	{
> +		.multiplier	= 15,
> +		.regval		= 0x08,
> +	},
> +	{
> +		.multiplier	= 20,
> +		.regval		= 0x09,
> +	},
> +	{
> +		.multiplier	= 25,
> +		.regval		= 0x0a,
> +	}
> +};
> +
> +static const struct da850_sata_mpy_mapping *
> +da850_sata_get_mpy(unsigned int multiplier)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
> +		if (da850_sata_mpy_table[i].multiplier == multiplier)
> +			return &da850_sata_mpy_table[i];
> +
> +	return NULL;
> +}
> +
>  /*
>   * The multiplier needed for 1.5GHz PLL output.
>   *
> - * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
> - * frequency (which is used by DA850 EVM board) and may need to be changed
> - * if you would like to use this driver on some other board.
> + * This is the default value suitable for the 100MHz crystal frequency
> + * used by DA850 EVM board, which doesn't use DT.

As I said in a reply on another patch, it seems like it would be better 
to use a clock that represents the crystal and use clk_get_rate() and 
calculate the multiplier from that.

For example, we have done something like this in 
usb20_phy_clk_set_parent() in arch/arm/mach-davinci/usb-da8xx.c.

>   */
> -#define DA850_SATA_CLK_MULTIPLIER	7
> +#define DA850_SATA_CLK_MULTIPLIER_DEFAULT	15
>
>  static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
> -			    void __iomem *ahci_base)
> +			    void __iomem *ahci_base,
> +			    const struct da850_sata_mpy_mapping *mpy)
>  {
>  	unsigned int val;
>
> @@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
>  	val &= ~BIT(0);
>  	writel(val, pwrdn_reg);
>
> -	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
> +	val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
>  	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
>  	      SATA_PHY_ENPLL(1);
>
> @@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
>
>  static int ahci_da850_probe(struct platform_device *pdev)
>  {
> +	const struct da850_sata_mpy_mapping *mpy;
>  	struct device *dev = &pdev->dev;
>  	struct ahci_host_priv *hpriv;
> -	struct resource *res;
> +	unsigned int multiplier;
>  	void __iomem *pwrdn_reg;
> +	struct resource *res;
>  	int rc;
>
>  	hpriv = ahci_platform_get_resources(pdev);
> @@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
>  	if (!pwrdn_reg)
>  		goto disable_resources;
>
> -	da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
> +	rc = of_property_read_u32(dev->of_node,
> +				  "da850,clk_multiplier", &multiplier);
> +	if (rc)
> +		multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
> +
> +	mpy = da850_sata_get_mpy(multiplier);
> +	if (!mpy) {
> +		dev_err(dev, "invalid multiplier value: %u\n", multiplier);
> +		rc = -EINVAL;
> +		goto disable_resources;
> +	}
> +
> +	da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
>
>  	rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
>  				     &ahci_platform_sht);
>