From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kishon Vijay Abraham I Subject: Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver Date: Thu, 27 Feb 2014 12:17:20 +0530 Message-ID: <530EDF78.4050408@ti.com> References: <1393308882-3964-1-git-send-email-lho@apm.com> <1393308882-3964-2-git-send-email-lho@apm.com> <1393308882-3964-3-git-send-email-lho@apm.com> <1393308882-3964-4-git-send-email-lho@apm.com> <530DB713.6000701@ti.com> <530ED4EF.1060000@ti.com> <530EDC65.8060106@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Loc Ho Cc: "devicetree@vger.kernel.org" , Suman Tripathi , Arnd Bergmann , Jon Masters , "patches@apm.com" , linux-kernel@vger.kernel.org, Olof Johansson , Don Dutile , Tejun Heo , Tuan Phan , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org On Thursday 27 February 2014 12:11 PM, Loc Ho wrote: > Hi, > >>>>>>> + >>>>>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg, >>>>>>> + u32 indirect_data_reg, u32 addr, u32 data) >>>>>>> +{ >>>>>>> + u32 val; >>>>>>> + u32 cmd; >>>>>>> + >>>>>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK; >>>>>>> + cmd = CFG_IND_ADDR_SET(cmd, addr); >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK' >>>>>> should >>>>>> be set then it should be part of the second argument. From the macro >>>>>> 'CFG_IND_ADDR_SET' the first argument should be more like the current >>>>>> value >>>>>> present in the register right? I feel the macro (CFG_IND_ADDR_SET) is >>>>>> not >>>>>> used in the way it is intended to. >>>>> >>>>> >>>>> >>>>> The macro XXX_SET is intended to update an field within the register. >>>>> The update field is returned. The first assignment lines are setting >>>>> another field. Those two lines can be written as: >>>>> >>>>> cmd = 0; >>>>> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit >>>>> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit >>>>> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR >>>> >>>> >>>> >>>> #define CFG_IND_ADDR_SET(dst, src) \ >>>> (((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0)) >>>> >>>> From this macro the first argument should be the present value in that >>>> register. Here you reset the address bits and write the new address bits. >>> >>> >>> Yes.. This is correct. I am clearing x number of bit and then set new >>> value. >>> >>>> IMO the first argument should be the value in 'csr_base + >>>> indirect_cmd_reg'. >>>> So it resets the address bits in 'csr_base + indirect_cmd_reg' and write >>>> down the new address bits. >>> >>> >>> Yes.. The above code does just that. In addition, I am also setting >>> the bits CFG_IND_WR_CMD_MASK and CFG_IND_CMD_DONE_MASK with the two >>> previous statement. Think of the code flow as follow: >>> >>> val = readl(some void * address); /* read the register */ >> >> >> Where are you reading the register in your code (before CFG_IND_ADDR_SET)? > > I am not reading the register as I will be completely setting them. Ok. Never-mind then. Sorry for the noise. You code is fine. Thanks Kishon