[U-Boot] [PATCH v4 15/28] arm: socfpga: combine clrbits/setbits into a single clrsetbits

[Ley Foon Tan]

Hi Marek

On Mon, Jan 23, 2017 at 11:58 AM, Marek Vasut <marex at denx.de> wrote:
>
> On 01/10/2017 06:20 AM, Chee Tien Fong wrote:
> > From: Tien Fong Chee <tien.fong.chee at 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 <dinguyen at opensource.altera.com>
> > Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
> > Cc: Marek Vasut <marex at denx.de>
> > Cc: Dinh Nguyen <dinguyen at kernel.org>
> > Cc: Chin Liang See <chin.liang.see at intel.com>
> > Cc: Tien Fong <skywindctf at 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.

 #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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot