From: "ext Kanigeri, Hari" <[email protected]>
Subject: RE: [PATCH 4/4][v3] OMAP:iommu- add TLB preservation support
Date: Thu, 22 Apr 2010 02:19:35 +0200
>> > @@ -241,6 +238,11 @@ int load_iotlb_entry(struct iommu *obj, struct
>> iotlb_entry *e)
>> > break;
>> > }
>> >
>> > + if (l.base == obj->nr_tlb_entries) {
>> > + dev_dbg(obj->dev, "%s: preserve entries full\n", __func__);
>> > + err = -EBUSY;
>>
>> I am afraid that this doesn't work. There are two cases where TLB
>> entries are set/updated:
>>
>> 1) H/W TWL always updates TLB entries automatically, IOW, increments
>> "vict" implicitly.
>> 2) A client module adds preservation TLB entry with explictly calling
>> "load_iotlb_entry()". IOW, increments both "vict" and "base"
>> intentionally.
>>
>> Because of 1), "vict" gets full soon and this is normal, but even if
>> this "vict" is full, a preservation entry "base" should be able to be
>> set by a client module unless "base" itself rearches full. IOW, Even
>> if all TLB entries are valid, a new preservation TLB entry should be
>> set by replacing one of the existing normal entry.
>>
>> In the case of "e" with perservation bit , "load_iotlb_entry()"
>> doesn't usually fail because it replaces the exisiting normal entry,
>> but it would fail only if all TLB entries are preserved.
>>
>
> -- I agree with your comments. My implementation was under the
> assumption that the User locks the TLB entries only at startup time,
> but as you mentioned this would not work during run time as the
> entries would already be having valid entries. DSPBridge and the
> OMAP4 IPCs both locks the entries at init time.
I understand your cases, but the implementation should cover more
general cases too.
> The following changes in load_tlb_entry function should handle the
> run time case too. This change would lock an entry irrespective of
> existing entry is valid or not.
> Please let me know if you agree.
OK for me, just small comment inlined.
> @@ -230,22 +227,32 @@ int load_iotlb_entry(struct iommu *obj, struct
> iotlb_entry *e)
>
> clk_enable(obj->clk);
>
> - for (i = 0; i < obj->nr_tlb_entries; i++) {
> - struct cr_regs tmp;
> -
> - iotlb_lock_get(obj, &l);
> - l.vict = i;
> - iotlb_lock_set(obj, &l);
> - iotlb_read_cr(obj, &tmp);
> - if (!iotlb_cr_valid(&tmp))
> - break;
> - }
> -
> - if (i == obj->nr_tlb_entries) {
> - dev_dbg(obj->dev, "%s: full: no entry\n", __func__);
> + iotlb_lock_get(obj, &l);
> + if (l.base == obj->nr_tlb_entries) {
> + dev_dbg(obj->dev, "%s: preserve entries full\n", __func__);
> err = -EBUSY;
dev_warn()?
I guess that it might be better to inform this a bit louder since all
preservation entries are managed well by a client.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
