2010/12/21 Anton Vorontsov <cbouatmailru@xxxxxxxxx>:
> On Sun, Dec 05, 2010 at 12:43:41AM +0800, mkl0301@xxxxxxxxx wrote:
> [...]
>
> Thanks for the patch!
>
> There are few issues though.
>
>> +static int cns3xxx_ahci_init(struct device *dev, void __iomem *addr)
>> +{
>> + u32 tmp;
>> +
>> + DPRINTK("ENTER\n");
>> +
>> + tmp = __raw_readl(MISC_SATA_POWER_MODE);
>> + tmp |= 0x1 << 16; /* Disable SATA PHY 0 from SLUMBER Mode */
>> + tmp |= 0x1 << 17; /* Disable SATA PHY 1 from SLUMBER Mode */
>> + __raw_writel(tmp, MISC_SATA_POWER_MODE);
>> +
>> + /* Enable SATA PHY */
>> + cns3xxx_pwr_power_up(0x1 << PM_PLL_HM_PD_CTRL_REG_OFFSET_SATA_PHY0);
>> + cns3xxx_pwr_power_up(0x1 << PM_PLL_HM_PD_CTRL_REG_OFFSET_SATA_PHY1);
>> +
>> + /* Enable SATA Clock */
>> + cns3xxx_pwr_clk_en(0x1 << PM_CLK_GATE_REG_OFFSET_SATA);
>> +
>> + /* De-Asscer SATA Reset */
>> + cns3xxx_pwr_soft_rst(CNS3XXX_PWR_SOFTWARE_RST(SATA));
>> +
>> + return 0;
>> +}
>> +
> [...]
>> -void __init cns3xxx_ahci_init(void)
>> -{
>> - u32 tmp;
>> -
>> - tmp = __raw_readl(MISC_SATA_POWER_MODE);
>> - tmp |= 0x1 << 16; /* Disable SATA PHY 0 from SLUMBER Mode */
>> - tmp |= 0x1 << 17; /* Disable SATA PHY 1 from SLUMBER Mode */
>> - __raw_writel(tmp, MISC_SATA_POWER_MODE);
>> -
>> - /* Enable SATA PHY */
>> - cns3xxx_pwr_power_up(0x1 << PM_PLL_HM_PD_CTRL_REG_OFFSET_SATA_PHY0);
>> - cns3xxx_pwr_power_up(0x1 << PM_PLL_HM_PD_CTRL_REG_OFFSET_SATA_PHY1);
>> -
>> - /* Enable SATA Clock */
>> - cns3xxx_pwr_clk_en(0x1 << PM_CLK_GATE_REG_OFFSET_SATA);
>> -
>> - /* De-Asscer SATA Reset */
>> - cns3xxx_pwr_soft_rst(CNS3XXX_PWR_SOFTWARE_RST(SATA));
>> -
>> - platform_device_register(&cns3xxx_ahci_pdev);
>> -}
>
> This is not good because cns3xxx_pwr_*() calls aren't thread-safe.
> We must use them only from the "single-threaded" platform code,
> i.e. very early.
> Once we add proper clocks (clkapi) and power management (regulators)
> support for CNS3xxx, we may move this into ahci->init() callback.
Almost every device driver needs to change those power/clk/reset bits,
and it's strange to enable everything on init phase. BTW, the EHCI
and OHCI that I sent previously also used these cns3xxx_pwr_*() calls.
Would they be supported in near future? If not, I would like look into those.
> Plus, unfortunately this patch breaks build when AHCI_PLATFORM is
> set to =m.
> arch/arm/mach-cns3xxx/built-in.o: In function 'cns3xxx_ahci_softreset':
> cns3420vb.c:(.text+0x36c): undefined reference to 'ahci_do_softreset'
> cns3420vb.c:(.text+0x390): undefined reference to 'ahci_do_softreset'
> cns3420vb.c:(.text+0x3a0): undefined reference to 'ahci_check_ready'
> arch/arm/mach-cns3xxx/built-in.o:(.data+0x35c): undefined reference to 'ahci_ops'
> make: *** [.tmp_vmlinux1] Error 1
Sorry that I didn't notice this. I was trying to reuse ahci-platform
as possible.
Building platform device as module seems strange to me, therefore I
would add a new ahci-platform alike platform driver.
Best Regards,
Mac Lin
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
