From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Nelson Date: Sun, 25 Mar 2012 08:33:18 -0700 Subject: [U-Boot] [PATCH V2] i.MX6: mx6q_sabrelite: add SATA bindings In-Reply-To: <4F6EF0CA.6050002@denx.de> References: <1331657978-19270-1-git-send-email-eric.nelson@boundarydevices.com> <4F6D7319.5050204@googlemail.com> <4F6D838B.6050509@denx.de> <4F6E363D.8080804@boundarydevices.com> <4F6EF0CA.6050002@denx.de> Message-ID: <4F6F3ABE.7030903@boundarydevices.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 Nelson >>>> >>>> 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 >>>>> >>>>> >>>>> >>>>> + 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.