On 02/16/2017 04:34 AM, Ley Foon Tan wrote: > Hi Marek > > On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut <ma...@denx.de> wrote: >> >> On 01/10/2017 06:20 AM, Chee Tien Fong wrote: >>> From: Tien Fong Chee <tien.fong.c...@intel.com> >>> >>> There is no dependency on doing a separate clrbits first in the >>> dwmac_deassert_reset function. Combine them into a single >>> clrsetbits call. >>> >>> Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com> >>> Signed-off-by: Tien Fong Chee <tien.fong.c...@intel.com> >>> Cc: Marek Vasut <ma...@denx.de> >>> Cc: Dinh Nguyen <dingu...@kernel.org> >>> Cc: Chin Liang See <chin.liang....@intel.com> >>> Cc: Tien Fong <skywind...@gmail.com> >>> --- >>> arch/arm/mach-socfpga/misc.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c >>> index 2645129..c97caea 100644 >>> --- a/arch/arm/mach-socfpga/misc.c >>> +++ b/arch/arm/mach-socfpga/misc.c >>> @@ -100,13 +100,10 @@ static void dwmac_deassert_reset(const unsigned int >>> of_reset_id, >>> return; >>> } >>> >>> - /* Clearing emac0 PHY interface select to 0 */ >>> - clrbits_le32(&sysmgr_regs->emacgrp_ctrl, >>> - SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift); >>> - >>> /* configure to PHY interface select choosed */ >>> - setbits_le32(&sysmgr_regs->emacgrp_ctrl, >>> - phymode << physhift); >> >> I don't think this patch is correct. The purpose of using these calls >> separately is so that the write with cleared physel mask actually >> reaches hardware, followed by read and another write with the physel >> mask set. clrsetbits will do only one read-modify-write cycle. > > The cleared physel mask is OR with the phymode set and then write to > hardware. So, I think the > result is the same.
The result isn't the same, look at how setbits_le32() is implemented. The previous code will perform two read-modify-writes, while clrsetbits_le32() will perform one read-modify-write. I dunno how the hardware is implemented internally, but there might be a reason why the original code contained two writes. > #define clrsetbits(type, addr, clear, set) \ > out_##type((addr), (in_##type(addr) & ~(clear)) | (set)) > >> >>> + clrsetbits_le32(&sysmgr_regs->emacgrp_ctrl, >>> + SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << physhift, >>> + phymode << physhift); >>> >>> /* Release the EMAC controller from reset */ >>> socfpga_per_reset(reset, 0); >>> >> >> >> -- >> Best regards, >> Marek Vasut >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot