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