From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751163AbaB0GsF (ORCPT ); Thu, 27 Feb 2014 01:48:05 -0500 Received: from arroyo.ext.ti.com ([192.94.94.40]:40131 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750748AbaB0GsE (ORCPT ); Thu, 27 Feb 2014 01:48:04 -0500 Message-ID: <530EDF78.4050408@ti.com> Date: Thu, 27 Feb 2014 12:17:20 +0530 From: Kishon Vijay Abraham I User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 MIME-Version: 1.0 To: Loc Ho CC: Tejun Heo , Olof Johansson , Arnd Bergmann , , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Don Dutile , Jon Masters , "patches@apm.com" , Tuan Phan , Suman Tripathi Subject: Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver 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> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@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