On Tue, 15 Oct 2024, Shyam Sundar S K wrote:
> On 10/14/2024 23:24, Ilpo Järvinen wrote:
> > On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> >> On 10/14/2024 13:26, Ilpo Järvinen wrote:
> >>> On Mon, 14 Oct 2024, Shyam Sundar S K wrote:
> >>>
> >>>> Use platform_get_resource() to fetch the memory resource instead of
> >>>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
> >>>> resources.
> >>>>
> >>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
> >>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
> >>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> >>>> ---
> >>>> drivers/platform/x86/amd/pmf/Kconfig | 1 +
> >>>> drivers/platform/x86/amd/pmf/acpi.c | 37 ++++++++-------------------
> >>>> drivers/platform/x86/amd/pmf/pmf.h | 6 +++--
> >>>> drivers/platform/x86/amd/pmf/tee-if.c | 8 +++---
> >>>> 4 files changed, 20 insertions(+), 32 deletions(-)
> >>>>
> >>>> + if (!pmf_dev->res) {
> >>>> + dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> >>>> + return -EINVAL;
> >>>> }
> >>>>
> >>>> - return AE_OK;
> >>>> -}
> >>>> + pmf_dev->policy_addr = pmf_dev->res->start;
> >>>> + pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
> >>>
> >>> If you have a resource, why you convert it into something custom like
> >>> this?
> >>>
> >>
> >> I will address your comments in v2. But for this specific comment:
> >>
> >> the DSDT looks like this:
> >>
> >> Device (PMF)
> >> {
> >> ...
> >>
> >> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> >> {
> >> Name (RBUF, ResourceTemplate ()
> >> {
> >> QWordMemory (ResourceConsumer, PosDecode, MinNotFixed, MaxNotFixed,
> >> NonCacheable, ReadOnly,
> >> 0x0000000000000000, // Granularity
> >> 0x000000FD00BC1000, // Range Minimum
> >> 0x000000FD00C0C000, // Range Maximum
> >> 0x0000000000000000, // Translation Offset
> >> 0x000000000004B000, // Length
> >> ,, , AddressRangeMemory, TypeStatic)
> >> }
> >>
> >> ...
> >> }
> >> }
> >>
> >> But, resource_size() will do 'res->end - res->start + 1;'
> >>
> >> So, that will become 0x4B000 + 1 = 0x4B001 which will exceed
> >> POLICY_BUF_MAX_SZ.
> >
> > That +1 is there to counter the -1 done on the set side. res->end is
> > supposed to point last valid address of the resource, not the address
> > after it. With round sizes, the end address is expected to end with lots
> > of Fs (hex) but in your example it ends into zeros (if I interpret your
> > numbers right)?
> >
>
> Yes, that's right.
>
>
> Couple of more examples, where resource_size() will fit correctly.
>
>
> Example #1:
> WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
> 0x0000, // Granularity
> 0x0D00, // Range Minimum
> 0x0FFF, // Range Maximum
> 0x0000, // Translation Offset
> 0x0300, // Length
> ,, , TypeStatic, DenseTranslation)
>
> resource_size() will do 'res->end - res->start + 1;'
> So,
> 0xFFF-0xD00 = 0x2FF
> 0x2FF + 1 = 0x300 (which matches the length field)
>
> Example #2:
> DWordMemory (ResourceProducer, SubDecode, MinFixed, MaxFixed,
> NonCacheable, ReadWrite,
> 0x00000000, // Granularity
> 0xFED45000, // Range Minimum
> 0xFED811FF, // Range Maximum
> 0x00000000, // Translation Offset
> 0x0003C200, // Length
> ,, , AddressRangeMemory, TypeStatic)
>
> Here,
> 0xFED811FF - 0xFED45000 = 0x3C1FF
> 0x3C1FF + 1 = 0x3C200 (which again matches the length field)
>
> But the same resource_size() will not help in case of PMF _CRS
> ResourceTemplate(). Hence I had to make a custom change like doing "-1".
>
> So, in the current change proposed here, we can have two options:
>
> 1) just use:
> pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
Since it doesn't seem to cover the entire resource, I think this option 1)
is better for now.
> 2) use resource_size() with -1 and leave a note on why "-1" is required
> pmf_dev->policy_sz = resource_size(pmf_dev->res) - 1;
>
> Let me know your thoughts.
--
i.
