Re: platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 18-Nov-24 4:55 PM, Andy Shevchenko wrote:
> On Mon, Nov 18, 2024 at 02:35:44PM +0000, Daniel Walker (danielwa) wrote:
>> On Mon, Nov 18, 2024 at 03:49:32PM +0200, Andy Shevchenko wrote:
>>> On Mon, Nov 18, 2024 at 01:32:55PM +0000, Daniel Walker (danielwa) wrote:
>>>> On Mon, Nov 18, 2024 at 03:24:20PM +0200, Andy Shevchenko wrote:
>>>>> On Mon, Nov 18, 2024 at 12:40:16PM +0000, Daniel Walker (danielwa) wrote:
> 
> ...
> 
>>>>> Are you referring to LPC GPIO?
>>>>  
>>>>  I don't know the hardware well enough to say for certain. It's whatever device 8086:19dd is.
>>>
>>> This is device which represents p2sb. It's not a GPIO device you are talking
>>> about for sure. You can send privately more detailed info in case this is shouldn't
>>> be on public to me to understand what would be the best approach to fix your issue.
>>
>> Here's a comment,
>>
>> /* INTEL Denverton GPIO registers are accessible using SBREG_BAR(bar 0) as base */
>>
>> We have gpio wired to an FPGA and I believe the gpio line is used to reset the
>> fpga.
>>
>> So the pci resources attached to 8086:19dd can be used to access gpio of some
>> type. 
>>
>> I'm not a pci expert but on the 19bb device bar 0 we use the below offset to manipulate
>> the gpio,
>>
>> #define INTEL_GPIO_REG_RESET_OFFSET          0xC50578
>>
>> The comments suggest this is gpio 6. I would seems your reaction would be that
>> there is no gpio on the 19dd device. Maybe our driver is access gpio thru p2sb
>> or something like that.
>>
>> Does the offset above make sense to you in the context of the p2sb ?
> 
> Yes, everything makes sense. Please, enable lpc_ich driver and forget about
> talking to the p2sb memory mapped IO.
> 
> /* Offset data for Denverton GPIO controllers */
> static const resource_size_t dnv_gpio_offsets[DNV_GPIO_NR_RESOURCES] = {
> 	[DNV_GPIO_NORTH] = 0xc20000,
> 	[DNV_GPIO_SOUTH] = 0xc50000,
> };
> 
> So, you are using a pin from the Community "South" of the on-die Denverton GPIO.
> 
> In EDS this called GPIO_6, while in the driver it's pin 88, i.e. SMB3_IE0_DATA.
> 
> You will need to
> - enable lpc_ich driver (CONFIG_LPC_ICH)
> - enable Intel Denverton pin control driver (CONFIG_PINCTRL_DENVERTON)
> - replace your custom approach to:
>   - GPIO lookup table
>   - proper GPIO APIs, as gpiod_get() or alike
> 
> See how it was done in the current kernel code:
> 
> 8241b55f1ded ("drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back")
> a6c80bec3c93 ("leds: simatic-ipc-leds-gpio: Add GPIO version of Siemens driver")
> 
> Hans, there will be no need to fix anything if they implement correct access
> to the GPIO, i.e. via driver and board code with GPIO lookup tables.

Agreed, still I'm not sure how I feel about us hiding the previously unhidden P2SB.

OTOH I guess it may have only been unhidden in the BIOS to make the hack they
are using possible in the first place.

Regards,

Hans