Re: [RFC PATCH 2/3] PCI: aardvark: Add support for sending Set_Slot_Power_Limit message

[Pali Rohár]

On Tuesday 24 August 2021 10:58:31 Rob Herring wrote:
> On Fri, Aug 20, 2021 at 06:00:22PM +0200, Pali Rohár wrote:
> > According to PCIe Base specification 3.0, when transitioning from a
> > non-DL_Up Status to a DL_Up Status, the Port must initiate the
> > transmission of a Set_Slot_Power_Limit Message to the other component
> > on the Link to convey the value programmed in the Slot Power Limit
> > Scale and Value fields of the Slot Capabilities register. This
> > Transmission is optional if the Slot Capabilities register has not
> > yet been initialized.
> > 
> > As PCIe Root Bridge is emulated by kernel emulate readback of Slot Power
> > Limit Scale and Value bits in Slot Capabilities register.
> > 
> > Also send that Set_Slot_Power_Limit message via Message Generation Control
> > Register in Link Up handler when link changes from down to up state and
> > slot power limit value was defined.
> > 
> > Slot power limit value is read from DT property 'slot-power-limit' which
> > value is in mW unit. When this DT property is not specified then it is
> > treated as "Slot Capabilities register has not yet been initialized".
> > 
> > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/pci/aardvark-pci.txt  |  2 +
> >  drivers/pci/controller/pci-aardvark.c         | 66 ++++++++++++++++++-
> >  2 files changed, 67 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
> > index 2b8ca920a7fa..bb658f261db0 100644
> > --- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
> > +++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
> > @@ -20,6 +20,7 @@ contain the following properties:
> >     define the mapping of the PCIe interface to interrupt numbers.
> >   - bus-range: PCI bus numbers covered
> >   - phys: the PCIe PHY handle
> > + - slot-power-limit: see pci.txt
> >   - max-link-speed: see pci.txt
> >   - reset-gpios: see pci.txt
> >  
> > @@ -52,6 +53,7 @@ Example:
> >  				<0 0 0 3 &pcie_intc 2>,
> >  				<0 0 0 4 &pcie_intc 3>;
> >  		phys = <&comphy1 0>;
> > +		slot-power-limit: <10000>;
> >  		pcie_intc: interrupt-controller {
> >  			interrupt-controller;
> >  			#interrupt-cells = <1>;
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index f94898f6072f..cf704c199c15 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -166,6 +166,11 @@
> >  #define VENDOR_ID_REG				(LMI_BASE_ADDR + 0x44)
> >  #define DEBUG_MUX_CTRL_REG			(LMI_BASE_ADDR + 0x208)
> >  #define     DIS_ORD_CHK				BIT(30)
> > +#define PME_MSG_GEN_CTRL			(LMI_BASE_ADDR + 0x220)
> > +#define     SEND_SET_SLOT_POWER_LIMIT		BIT(13)
> > +#define     SEND_PME_TURN_OFF			BIT(14)
> > +#define     SLOT_POWER_LIMIT_DATA_SHIFT		16
> > +#define     SLOT_POWER_LIMIT_DATA_MASK		GENMASK(25, 16)
> >  
> >  /* PCIe core controller registers */
> >  #define CTRL_CORE_BASE_ADDR			0x18000
> > @@ -267,6 +272,7 @@ static bool advk_pcie_link_up(struct advk_pcie *pcie)
> >  {
> >  	u32 val, ltssm_state;
> >  	u16 slotsta, slotctl;
> > +	u32 slotpwr;
> >  	bool link_up;
> >  
> >  	val = advk_readl(pcie, CFG_REG);
> > @@ -277,7 +283,25 @@ static bool advk_pcie_link_up(struct advk_pcie *pcie)
> >  		pcie->link_up = true;
> >  		slotsta = le16_to_cpu(pcie->bridge.pcie_conf.slotsta);
> >  		slotctl = le16_to_cpu(pcie->bridge.pcie_conf.slotctl);
> > +		slotpwr = (le32_to_cpu(pcie->bridge.pcie_conf.slotcap) &
> > +			   (PCI_EXP_SLTCAP_SPLV | PCI_EXP_SLTCAP_SPLS)) >> 7;
> >  		pcie->bridge.pcie_conf.slotsta = cpu_to_le16(slotsta | PCI_EXP_SLTSTA_DLLSC);
> > +		if (!(slotctl & PCI_EXP_SLTCTL_ASPL_DISABLE) && slotpwr) {
> > +			/*
> > +			 * According to PCIe Base specification 3.0, when transitioning from a
> > +			 * non-DL_Up Status to a DL_Up Status, the Port must initiate the
> > +			 * transmission of a Set_Slot_Power_Limit Message to the other component
> > +			 * on the Link to convey the value programmed in the Slot Power Limit
> > +			 * Scale and Value fields of the Slot Capabilities register. This
> > +			 * Transmission is optional if the Slot Capabilities register has not
> > +			 * yet been initialized.
> > +			 */
> > +			val = advk_readl(pcie, PME_MSG_GEN_CTRL);
> > +			val &= ~SLOT_POWER_LIMIT_DATA_MASK;
> > +			val |= slotpwr << SLOT_POWER_LIMIT_DATA_SHIFT;
> > +			val |= SEND_SET_SLOT_POWER_LIMIT;
> > +			advk_writel(pcie, val, PME_MSG_GEN_CTRL);
> > +		}
> >  		if ((slotctl & PCI_EXP_SLTCTL_DLLSCE) && (slotctl & PCI_EXP_SLTCTL_HPIE))
> >  			mod_timer(&pcie->link_up_irq_timer, jiffies + 1);
> >  	}
> > @@ -956,6 +980,9 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
> >  static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> >  {
> >  	struct pci_bridge_emul *bridge = &pcie->bridge;
> > +	struct device *dev = &pcie->pdev->dev;
> > +	u8 slot_power_limit_scale, slot_power_limit_value;
> > +	u32 slot_power_limit;
> >  	int ret;
> >  
> >  	bridge->conf.vendor =
> > @@ -988,6 +1015,40 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> >  	/* Indicates supports for Completion Retry Status */
> >  	bridge->pcie_conf.rootcap = cpu_to_le16(PCI_EXP_RTCAP_CRSVIS);
> >  
> 
> > +	if (of_property_read_u32(dev->of_node, "slot-power-limit", &slot_power_limit))
> > +		slot_power_limit = 0;
> > +
> > +	if (slot_power_limit)
> > +		dev_info(dev, "Slot power limit %u.%uW\n", slot_power_limit / 1000,
> > +			 (slot_power_limit / 100) % 10);
> 
> Add a wrapper function for this.
> 
> > +
> > +	/* Calculate Slot Power Limit Value and Slot Power Limit Scale */
> > +	if (slot_power_limit == 0) {
> > +		slot_power_limit_scale = 0;
> > +		slot_power_limit_value = 0x00;
> > +	} else if (slot_power_limit <= 255) {
> > +		slot_power_limit_scale = 3;
> > +		slot_power_limit_value = slot_power_limit;
> > +	} else if (slot_power_limit <= 255*10) {
> > +		slot_power_limit_scale = 2;
> > +		slot_power_limit_value = slot_power_limit / 10;
> > +	} else if (slot_power_limit <= 255*100) {
> > +		slot_power_limit_scale = 1;
> > +		slot_power_limit_value = slot_power_limit / 100;
> > +	} else if (slot_power_limit <= 239*1000) {
> > +		slot_power_limit_scale = 0;
> > +		slot_power_limit_value = slot_power_limit / 1000;
> > +	} else if (slot_power_limit <= 250*1000) {
> > +		slot_power_limit_scale = 0;
> > +		slot_power_limit_value = 0xF0;
> > +	} else if (slot_power_limit <= 275*1000) {
> > +		slot_power_limit_scale = 0;
> > +		slot_power_limit_value = 0xF1;
> > +	} else {
> > +		slot_power_limit_scale = 0;
> > +		slot_power_limit_value = 0xF2;
> > +	}
> 
> This all looks like it should be common code.

Yes, this is common code. I will put it into some common function.

> > +
> >  	/*
> >  	 * Mark bridge as Hot-Plug Capable to allow delivering Data Link Layer
> >  	 * State Changed interrupt. No Command Completed Support is set because
> > @@ -996,7 +1057,10 @@ static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
> >  	 * bit permanently as there is no support for unplugging PCIe card from
> >  	 * the slot. Assume that PCIe card is always connected in slot.
> >  	 */
> > -	bridge->pcie_conf.slotcap = cpu_to_le32(PCI_EXP_SLTCAP_NCCS | PCI_EXP_SLTCAP_HPC);
> > +	bridge->pcie_conf.slotcap = cpu_to_le32(PCI_EXP_SLTCAP_NCCS |
> > +						PCI_EXP_SLTCAP_HPC |
> 
> While not new, how does the driver know the board is hotplug capable. 
> IIRC, we have a property for that.

Just ignore it for now. It is part of unfinished patches. I sent it to
show how slot_power_limit would be configured via emulated root bridge.

> > +						slot_power_limit_value << 7 |
> > +						slot_power_limit_scale << 15);
> >  	bridge->pcie_conf.slotsta = cpu_to_le16(PCI_EXP_SLTSTA_PDS);
> >  
> >  	return 0;
> > -- 
> > 2.20.1
> > 
> >