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

[Andrew Murray]

On Tue, Oct 22, 2019 at 05:20:00PM +0800, Dilip Kota wrote:
> Hi Andrew Murray,
> 
> On 10/21/2019 9:38 PM, Andrew Murray wrote:
> > On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote:
> > > PCIe RC driver on Intel Gateway SoCs have a requirement
> > > of changing link width and speed on the fly.
> > > So add the sysfs attributes to show and store the link
> > > properties.
> > > Add the respective link resize function in pcie DesignWare
> > > framework so that Intel PCIe driver can use during link
> > > width configuration on the fly.
> > > 
> > > Signed-off-by: Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx>

> > > +/*
> > > + * 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. 

Thanks,

Andrew Murray

> 
> Regards,
> Dilip
> 
> > 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > > +ATTRIBUTE_GROUPS(pcie_cfg);
> > > +
> > > +static int intel_pcie_sysfs_init(struct intel_pcie_port *lpp)
> > > +{
> > > +	return devm_device_add_groups(lpp->pci.dev, pcie_cfg_groups);
> > > +}
> > > +
> > >   static void __intel_pcie_remove(struct intel_pcie_port *lpp)
> > >   {
> > >   	intel_pcie_core_irq_disable(lpp);
> > > @@ -490,8 +591,17 @@ static int intel_pcie_rc_init(struct pcie_port *pp)
> > >   {
> > >   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > >   	struct intel_pcie_port *lpp = dev_get_drvdata(pci->dev);
> > > +	int ret;
> > > -	return intel_pcie_host_setup(lpp);
> > > +	ret = intel_pcie_host_setup(lpp);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = intel_pcie_sysfs_init(lpp);
> > > +	if (ret)
> > > +		__intel_pcie_remove(lpp);
> > > +
> > > +	return ret;
> > >   }
> > >   int intel_pcie_msi_init(struct pcie_port *pp)
> > > -- 
> > > 2.11.0
> > >