Messages in this thread | | | Date | Thu, 27 Feb 2014 12:17:20 +0530 | From | Kishon Vijay Abraham I <> | Subject | Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver |
| |
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
| |