Re: [PATCH 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4472 bytes --]

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@amd.com>
> >>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >>>> ---
> >>>>  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.