Hi Stefano,
On 03/25/2012 03:17 AM, Stefano Babic wrote:
On 24/03/2012 22:01, Eric Nelson wrote:
Hi Stefano,
On 03/24/2012 01:19 AM, stefano babic wrote:
Am 24/03/2012 08:09, schrieb Dirk Behme:
Hi Stefano,
Hi Dirk,
On 13.03.2012 17:59, Eric Nelson wrote:
Signed-off-by: Eric Nelsoneric.nelson@boundarydevices.com
Should this patch go into your u-boot-imx.git -next?
mmhh...in patchworks this is marked as "Changes requested" - I do not remember now why, but I have a couple of open questions rereading the patch...
Funny how that happens with time.
Well, it is not a case ;-). I scan regularly patchwork and I check for not worked patches - however, if a patch is in "Changes requested", I expect a new version and I do not care about it. Dirk's message let me convince that the patch was not deeply reviewed and I checked it again. So many thanks to Dirk.
Ack that thanks to Dirk!
diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index 1d09a72..5915159 100644 --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
<snip>
- if (timeout<= 0)
return -1;- reg&= ~BM_ANADIG_PLL_SYS_BYPASS;
- writel(reg,&imx_ccm->analog_pll_enet);
- reg |= BM_ANADIG_PLL_ENET_ENABLE_SATA;
- writel(reg,&imx_ccm->analog_pll_enet);
Is it all this part really board specific or mx6 specific ? I would like to split this part into a common and a board specific parts, and making the common part available for other boards.
This is all generic MX6 SATA enablement and could be moved into common code.
Is cpu/armv7/mx6/clock.c the right place?
How about routine named 'enable_sata_clock()'?
ok, agree.
Will do.
- /* Enable sata phy */
- reg = readl(&iomuxc_regs->gpr[13])
+& (IOMUXC_GPR13_SDMA_STOP_REQ
|IOMUXC_GPR13_CAN2_STOP_REQ|IOMUXC_GPR13_CAN1_STOP_REQ|IOMUXC_GPR13_ENET_STOP_REQ);Why do you need to touch CAN bits ?
Not touching them, just keeping them the way they are and clearing out the rest (which are all SATA fields).
Then these three are superfluous, are not they ? You read the register, and putting in "or" with IOMUXC_GPR13_CANx_STOP_REQ does nothing.
Why do you not use clrsetbits to change the sata bits ?
There's a simple answer for this: I hadn't heard of 'clrsetbits' before.
Perhaps this would be clearer: reg = readl(&iomuxc->gpr[13]) & ~(IOMUXC_GPR13_SATA_PHY_8_MASK |IOMUXC_GPR13_SATA_PHY_7_MASK |IOMUXC_GPR13_SATA_PHY_6_MASK |IOMUXC_GPR13_SATA_SPEED_MASK |IOMUXC_GPR13_SATA_PHY_5_MASK |IOMUXC_GPR13_SATA_PHY_4_MASK |IOMUXC_GPR13_SATA_PHY_3_MASK |IOMUXC_GPR13_SATA_PHY_2_MASK |IOMUXC_GPR13_SATA_PHY_1_MASK);
Or better yet, maybe a macro in iomux-v3.h:
clrsetbits does exactly what you want, I think.
Ok. I'll address in a V2.