Hilman, Kevin wrote:
> "Cousson, Benoit" <b-cousson@xxxxxx> 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
Thanks for your comments.
I will work on the hwmod patch and submit it again.
Mansoor--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
