On 12/01/2015 12:29 PM, Arnd Bergmann wrote:
> On Tuesday 01 December 2015 11:14:57 Stanimir Varbanov wrote:
>> + if (srcs & BAM_IRQ) {
>> clr_mask = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_STTS));
>>
>> - /* don't allow reorder of the various accesses to the BAM registers
>> */
>> - mb();
>> + /*
>> + * don't allow reorder of the various accesses to the BAM
>> + * registers
>> + */
>> + mb();
>>
>> - writel_relaxed(clr_mask, bam_addr(bdev, 0, BAM_IRQ_CLR));
>> + writel_relaxed(clr_mask, bam_addr(bdev, 0, BAM_IRQ_CLR));
>> + }
>>
>
> I think the comment here should be moved: change the writel_relaxed()
> to writel(), which already includes the appropriate barriers, and
If we agree with such a change it should be subject to another patch.
> add a comment at the readl_relaxed() to explain why it doesn't need
> a barrier.
Infact I'm not sure that readl_relaxed(BAM_IRQ_STTS) does not need
barrier. If I read the code above correctly the mb() should guarantee
that all load and store operations before it are happened before the
write to BAM_IRQ_CLR register, and on the other hand if we replace
writel_relaxed with writel, the writel has wmb() which guarantees only
store operations. Did I miss something?
--
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
