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

[Dilip Kota <eswara.kota@linux.intel.com>]

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
>
>>