Re: [PATCH v7 2/2] PCI: keembay: Add support for Intel Keem Bay

["Krzysztof Wilczyński" <kw@linux.com>]

Hi Srikanth,

Thank you for all the work here!

> +config PCIE_KEEMBAY_HOST
> +	bool "Intel Keem Bay PCIe controller - Host mode"
> +	depends on ARCH_KEEMBAY || COMPILE_TEST
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	select PCIE_DW_HOST
> +	select PCIE_KEEMBAY
> +	help
> +	  Say 'Y' here to enable support for the PCIe controller in Keem Bay
> +	  to work in host mode.
> +	  The PCIe controller is based on Designware Hardware and uses
> +	  DesignWare core functions.
> +
> +config PCIE_KEEMBAY_EP
> +	bool "Intel Keem Bay PCIe controller - Endpoint mode"
> +	depends on ARCH_KEEMBAY || COMPILE_TEST
> +	depends on PCI && PCI_MSI_IRQ_DOMAIN
> +	depends on PCI_ENDPOINT
> +	select PCIE_DW_EP
> +	select PCIE_KEEMBAY
> +	help
> +	  Say 'Y' here to enable support for the PCIe controller in Keem Bay
> +	  to work in endpoint mode.
> +	  The PCIe controller is based on Designware Hardware and uses
> +	  DesignWare core functions.

It would be "DesignWare" in the sentences above.

[...]
> +static void keembay_ep_reset_deassert(struct keembay_pcie *pcie)
> +{
> +	/*
> +	 * Ensure that PERST# is asserted for a minimum of 100ms
[...]

Just a nitpick, so feel free to ignore, absolutely.  A missing period at
the end of the sentence.

> +static void keembay_pcie_ltssm_enable(struct keembay_pcie *pcie, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(pcie->apb_base + PCIE_REGS_PCIE_APP_CNTRL);
> +	if (enable)
> +		val |= APP_LTSSM_ENABLE;
> +	else
> +		val &= ~APP_LTSSM_ENABLE;
> +	writel(val, pcie->apb_base + PCIE_REGS_PCIE_APP_CNTRL);
> +}
>

Another nitpick, so also feel free to ignore, of course.

I wonder if calling this function keembay_pcie_set_ltssm or perhaps
keembay_pcie_ltssm_set would be more aligned with what it does, since it
turns the LTSSM support on and off, so to speak, as needed.  Do you
think this would be OK to do?

Krzysztof