Re: [PATCH] OMAP: TI816X: Add SATA support

[Kevin Hilman <khilman@ti.com>]

"Cousson, Benoit" <b-cousson@ti.com> writes:

> On 4/7/2011 1:23 PM, Balbi, Felipe wrote:
>> Hi,
>>
>> On Thu, Apr 07, 2011 at 03:54:47PM +0530, Basheer, Mansoor Ahamed wrote:
>>> @@ -100,6 +102,129 @@ static int __init omap4_l3_init(void)
>>>   }
>>>   postcore_initcall(omap4_l3_init);
>>>
>>> +#if defined(CONFIG_SATA_AHCI_PLATFORM) || \
>>> +	defined(CONFIG_SATA_AHCI_PLATFORM_MODULE)
>>> +
>>> +static struct ahci_platform_data omap_sata_pdata;
>>> +static u64 omap_sata_dmamask = DMA_BIT_MASK(32);
>>> +static struct clk *omap_sata_clk;
>>> +
>>> +/* SATA PHY control register offsets */
>>> +#define SATA_P0PHYCR_REG	0x178
>>> +#define SATA_P1PHYCR_REG	0x1F8
>>
>> prepend all with TI816X_
>>
>>> +#define SATA_PHY_ENPLL(x)	((x)<<  0)
>>> +#define SATA_PHY_MPY(x)		((x)<<  1)
>>> +#define SATA_PHY_LB(x)		((x)<<  5)
>>> +#define SATA_PHY_CLKBYP(x)	((x)<<  7)
>>> +#define SATA_PHY_RXINVPAIR(x)	((x)<<  9)
>>> +#define SATA_PHY_LBK(x)		((x)<<  10)
>>> +#define SATA_PHY_RXLOS(x)	((x)<<  12)
>>> +#define SATA_PHY_RXCDR(x)	((x)<<  13)
>>> +#define SATA_PHY_RXEQ(x)	((x)<<  16)
>>> +#define SATA_PHY_RXENOC(x)	((x)<<  20)
>>> +#define SATA_PHY_TXINVPAIR(x)	((x)<<  21)
>>> +#define SATA_PHY_TXCM(x)	((x)<<  22)
>>> +#define SATA_PHY_TXSWING(x)	((x)<<  23)
>>
>> the ones which are single bits, you define as:
>>
>> #define TI816X_SATA_PHY_TXINVPAIR	(1<<  21)
>>
>> or
>>
>> #define TI816X_SATA_PHY_TXINVPAIR	BIT(21)
>>
>>> +#define SATA_PHY_TXDE(x)	((x)<<  27)
>>> +
>>> +#define TI816X_SATA_BASE	0x4A140000
>>
>> you should probably define these on some header file. Also SATA_BASE
>> should be an increment to the global base.
>
> In fact, you should even use hwmod data for that.
>
>>> +
>>> +static int ti816x_ahci_plat_init(struct device *dev, void __iomem *base)
>>> +{
>>> +	unsigned int phy_val;
>>> +	int ret;
>>> +
>>> +	omap_sata_clk = clk_get(dev, NULL);
>>> +	if (IS_ERR(omap_sata_clk)) {
>>> +		pr_err("ahci : Failed to get SATA clock\n");
>>> +		return PTR_ERR(omap_sata_clk);
>>> +	}
>>
>> can't you use pm_runtime do achieve this ?
>>
>>> +
>>> +	if (!base) {
>>> +		pr_err("ahci : SATA reg space not mapped, PHY enable failed\n");
>>> +		ret = -ENOMEM;
>>> +		goto err;
>>> +	}
>>> +
>>> +	ret = clk_enable(omap_sata_clk);
>>> +	if (ret) {
>>> +		pr_err("ahci : Clock enable failed\n");
>>> +		goto err;
>>> +	}
>
> As Felipe suggested, pm_runtime is probably the right API for that.
> It should even potentially be done in the driver directly since it is
> a generic API and most platform should probably have to enable
> "something" at that time.

Just to second what Felipe and Benoit already said:

Much of what this patch does duplicates what would happen
"automatcially" if this was done using omap_hwmod/omap_device + runtime
PM.  For example, platform_device creation is automatic with hwmod data
+ omap_device build, and clock management is done by runtime PM.

Kevin