[PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Bharat Kumar Gogada]

> Ping
> 
> > Subject: RE: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for
> > Xilinx NWL PCIe Host Controller
> >
> >  > On Mon, Dec 21, 2015 at 05:23:47AM +0000, Bharat Kumar Gogada wrote:
> > > > Hi Bjorn, can you comment on this. Marc has also replied for query
> > > > on
> > > irq_dispose_mapping().
> > >
> > > I'm not sure exactly what you want me to comment on.
> > I wanted you to check for Marc's reply.
> > >
> > > > > Subject: RE: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support for
> > > > > Xilinx NWL PCIe Host Controller
> > > > >
> > > > > > Subject: Re: [PATCH v11] PCI: Xilinx-NWL-PCIe: Added support
> > > > > > for Xilinx NWL PCIe Host Controller
> > > > > >
> > > > > > [+cc Marc for irq_dispose_mapping() question]
> > > > > >
> > > > > > On Thu, Dec 10, 2015 at 02:10:34PM +0000, Bharat Kumar Gogada
> > > wrote:
> > > > > > I'm trying to figure out what the difference is between these
> > > > > > two checks and why you have both of them:
> > > > > >
> > > > > > > +	if (bus->number == pcie->root_busno && devfn > 0)
> > > > > > > +	if (bus->primary == pcie->root_busno && devfn > 0)
> > > > > >
> > > > > > If I understand correctly, pcie->root_busno is the bus number
> > > > > > of the Root Port device (likely 00).  I think the "bus->number
> > > > > > ==
> > > > > > pcie->root_busno && devfn > 0" check means that the Root Port,
> > > > > > pcie->e.g.,
> > > > > > 00:00.0, is the only device allowed on bus 00.  Often a Root
> > > > > > Complex contains several Root Ports and other integrated
> > > > > > devices that typically are
> > > > > on bus 00.
> > > > > > But in your case, I think you're saying there is only the
> > > > > > single Root Port and no other devices.
> > > > > >
> > > > > > I think that first check takes care of everything on bus 00,
> > > > > > so I'm trying to figure out what the second check is for.
> > > > > > Assume your Root Port is device
> > > > > > 00:00.0 and it is a bridge to [bus 01-ff].  Then we have two
> > > > > > pci_bus structs with these values:
> > > > > >
> > > > > >   bus->number = 00
> > > > > >   bus->primary = 00
> > > > > >   bus->busn_res = [bus 00-ff]
> > > > > >
> > > > > >   bus->number = 01
> > > > > >   bus->primary = 00
> > > > > >   bus->busn_res = [bus 01-ff]
> > > > > >
> > > > > > Because of the first check, 00:00.0 is the only possible
> > > > > > device on bus 00, and because of the second check, 01:00.0 is
> > > > > > the only possible device on
> > > > > bus 01.
> > > > > > Therefore, you don't support a multifunction device connected
> > > > > > to the Root Port.  Right?
> > > > > >
> > > > > We support multifunction devices also, so this check should not
> > > > > be there, will remove this check in next patch.
> > >
> > > It looks like you're planning to change this.
> > No, our core supports multifunction devices it was my
> > misunderstanding, due to which this condition was present.
> > > > > > > > > +		return false;
> > > > > > > > > +
> > > > > > > > > +	return true;
> > > > > > > > > +}
> > > > > > > > > + * nwl_setup_sspl - Set Slot Power limit
> > > > > > > > > + *
> > > > > > > > > + * @pcie: PCIe port information  */ static int
> > > > > > > > > +nwl_setup_sspl(struct nwl_pcie *pcie)
> > > > > > > >
> > > > > > > The Set_Slot_Power_Limit Message includes a one DW data
> > payload.
> > > > > > > The data payload is copied from the Slot Capabilities
> > > > > > > register of the Downstream Port and is written into the
> > > > > > > Device Capabilities register of the Upstream Port on the
> > > > > > > other side of the Link. Bits 9:8 of the data payload map to
> > > > > > > the Slot Power Limit Scale field and bits 7:0 map to the
> > > > > > > Slot Power Limit Value field. Bits 31:10 of the data payload
> > > > > > > must be set to all 0's by the Transmitter and ignored by the
> > > > > Receiver.
> > > > > >
> > > > > > > This Message is sent automatically by the Downstream Port
> > > > > > > (of a Root Complex or a Switch) when one of the following
> > > > > > > events
> > occurs:
> > > > > > > -> On a Configuration Write to the Slot Capabilities
> > > > > > > -> register (see
> > > > > > > Section 7.8.9) when the Data Link Layer reports DL_Up status.
> > > > > >
> > > > > > I interpret this as meaning "the *hardware* automatically
> > > > > > sends a Set_Slot_Power_Limit Message."  There's no mention of
> > > > > > software doing anything other than the configuration write.
> > > > > >
> > > > > > If your hardware doesn't do that, I think it's a defect.  It's
> > > > > > fine to work around it, but we should have a comment to that
> > > > > > effect so people don't copy the code to new drivers that don't
> > > > > > need
> > it.
> > > > >
> > > > > Our hardware is not capable of doing it, so we are doing it
> > > > > software. Yes I will add some comments.
> > >
> > > And add a comment here.
> > >
> > You meant to add comments for the function clearly mentioning that
> > hardware doesn't support this so doing it software ?
> >
> > > > > > It's a little strange that 7.8.9 talks about writing to this
> > > > > > register when all of its fields are HwInit and supposedly
> > > > > > read-only.  I had assumed devices would use strapping or
> > > > > > implementation-specific registers to set the Slot Power
> > > > > > values, but maybe some devices use direct
> > > > > writes to Slot Capabilities instead.
> > > > > >
> > > > > > BTW, I noticed a related lspci bug: it didn't decode the
> > > > > > Capture Slot Power Limit in Device Capabilities of Endpoints.
> > > > > > I posted a fix for that
> > > > > separately.
> > > > > >
> > > > > > The Slot Power Limit (in Slot Capabilities) indicates how much
> > > > > > power the slot can supply to a downstream device.  That's a
> > > > > > function of the platform design, so it seems like this is
> > > > > > something you want to set via DT or some other mechanism that
> > > > > > knows
> > > about the platform.
> > > > > > Intercepting all config writes and updating it with whatever
> > > > > > the caller supplies doesn't sound wise.  The value might be
> > > > > > coming from setpci or some other source with no knowledge of the
> platform.
> > > > >
> > > > > Agreed, but this is what can be done, it is difficult to
> > > > > determine who does what.
> > >
> > > Your driver is based on DT.  What prevents you from adding a DT
> > > property that shows the slot power capability?
> > >
> > You meant to add a DT property, Example: xlnx,slot_power_cap; as
> > required property, and use this property to call nwl_setup_sspl
> > function. Currently this function is being invoked in config write
> > function if the condition meets a write to slot power capabilities register.
> > > > > > > > > +			status = nwl_bridge_readl(pcie,
> > > > > TX_PCIE_MSG)
> > > > > > > > > +						  &
> MSG_DONE_BIT;
> > > > > > > > > +			if (status) {
> > > > > > > > > +				status =
> nwl_bridge_readl(pcie,
> > > > > > > > TX_PCIE_MSG)
> > > > > > > > > +						  &
> > > > > MSG_DONE_STATUS_BIT;
> > > > > >
> > > > > > > > It's not clear to me whether you need to re-read
> > > > > > > > TX_PCIE_MSG
> > > here.
> > > > > > >
> > > > > > > MSG_DONE_BIT qualifies MSG_DONE_STATUS_BIT; so value of
> > > > > > > MSG_DONE_STATUS_BIT is valid only when MSG_DONE_BIT = 1
> > > > > >
> > > > > > That doesn't answer the question of whether another read is
> > required.
> > > > > > In fact, I would argue that if MSG_DONE_STATUS_BIT is only
> > > > > > valid when MSG_DONE_BIT = 1, you *should* only do one read,
> > > > > > because
> > > you
> > > > > > want to capture both bits simultaneously so you know they're
> > > > > > consistent, e.g.,
> > > > > >
> > > > > >   status = nwl_bridge_readl(pcie, TX_PCIE_MSG);
> > > > > >   if (status & MSG_DONE_BIT) {
> > > > > >     if (status & MSG_DONE_STATUS_BIT)
> > > > > >       ...
> > > > > >   }
> > > > > >
> > > > > > If you read the register twice, you always have to worry about
> > > > > > what changes MSG_DONE_BIT, and how you guarantee that the
> > > second
> > > > > > read happens before MSG_DONE_BIT changes.
> > > > > >
> > > > > Agreed, will do it in this way, once will also confirm with IP
> > > > > owner regarding both bits being updated parallel.
> > >
> > > It sounds like you're working on resolving this.
> > >
> > > Did I miss a question for me?
> >
> > Since IP owner is different, we don't have exact information by that
> > time we commented, now we got Information from owner that both buts
> > simultaneously.
> >
> > Bharat
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the
> body of a message to majordomo at vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html