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


-- 
Best regards,
Marek Vasut