Re: [PATCH v4 3/3] pci: intel: Add sysfs attributes to configure pcie link

[Dilip Kota]

On 10/25/2019 5:34 PM, Andrew Murray wrote:

> > > > +/*
> > > > + * Link width change on the fly is not always successful.
> > > > + * It also depends on the partner.
> > > > + */
> > > > +static ssize_t pcie_width_store(struct device *dev,
> > > > +				struct device_attribute *attr,
> > > > +				const char *buf, size_t len)
> > > > +{
> > > > +	struct intel_pcie_port *lpp = dev_get_drvdata(dev);
> > > > +	unsigned long val;
> > > > +	int ret;
> > > > +
> > > > +	lpp = dev_get_drvdata(dev);
> > > > +
> > > > +	ret = kstrtoul(buf, 10, &val);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (val > lpp->max_width)
> > > > +		return -EINVAL;
> > > > +
> > > > +	/* HW auto bandwidth negotiation must be enabled */
> > > > +	pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0,
> > > > +			    PCIE_CAP_OFST + PCI_EXP_LNKCTL);
> > > > +	dw_pcie_link_width_resize(&lpp->pci, val);
> > > > +
> > > > +	return len;
> > > > +}
> > > > +static DEVICE_ATTR_WO(pcie_width);
> > > > +
> > > > +static struct attribute *pcie_cfg_attrs[] = {
> > > > +	&dev_attr_pcie_link_status.attr,
> > > > +	&dev_attr_pcie_speed.attr,
> > > > +	&dev_attr_pcie_width.attr,
> > > > +	NULL,
> > > > +};
> > > Is there a reason that these are limited only to the Intel driver and
> > > not the wider set of DWC drivers?
> > >
> > > Is there anything specific here about the Intel GW driver?
> > Yes, they need intel_pcie_max_speed_setup() and pcie_link_gen_to_str().
> > Once intel_pcie_max_speed_setup() moved to DesignWare framework (as per
> > Bjorn Helgaas inputs) and use pcie_link_speed[] array instead of
> > pcie_link_gen_to_str() (as per gustavo pimentel inputs) we can move this to
> > PCIe DesignWare framework or to pci sysfs file.
> I think the key concern here is this: If you introduce sysfs controls that
> represent generic PCI concepts (such as changing the link speed) - the concept
> isn't limited to a particular host controller, it's limited to PCI. Therefore
> the sysfs control should also apply more widely to all PCI controllers. This
> is important as otherwise you may end up getting a slightly different user
> interface to achieve the same consequence depending on the host-controller in
> use.
>
> If each controller driver has a different way of doing things, then it lends
> itself to having some set of ops that they can all implement. Or perhaps there
> is a middle-ground solution where this applies just to DWC devices and not all
> devices.
I see the better way is to move the implementation to PCIe DesignWare 
framework because of
the registers programming.

Regards,
Dilip
> Thanks,
>
> Andrew Murray
>
> >