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

[Marek Vasut]

On 02/16/2017 04:34 AM, Ley Foon Tan wrote:
> 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.

The original code does TWO writes into the hardware (one with zeroed
physel mask, the other with configured physel mask), the new code does
ONE write into the hardware. Why ? Is that because of some special
hardware property ? Or is one write really fine ?

>  #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


-- 
Best regards,
Marek Vasut