On Sun, Nov 18, 2018 at 10:52 PM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> On Sun, Nov 04 2018, Sergio Paracuellos wrote:
>
> > Add initialization of each PCIe port reading and initializing
> > data using device tree.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > ---
> > drivers/staging/mt7621-pci/pci-mt7621.c | 75 +++++++++++++++++++++++++++++++--
> > 1 file changed, 71 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> > index 8371a9c..b7cb273 100644
> > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > @@ -126,16 +126,20 @@ static int pcie_link_status;
> >
> > /**
> > * struct mt7621_pcie_port - PCIe port information
> > - * @base: IO mapped register base
> > + * @base: I/O mapped register base
> > * @list: port list
> > * @pcie: pointer to PCIe host info
> > - * @reset: pointer to port reset control
> > + * @pcie_rst: pointer to port reset control
> > + * @pcie_clk: PCIe clock
> > + * @slot: port slot
> > */
> > struct mt7621_pcie_port {
> > void __iomem *base;
> > struct list_head list;
> > struct mt7621_pcie *pcie;
> > - struct reset_control *reset;
> > + struct reset_control *pcie_rst;
> > + struct clk *pcie_clk;
> > + u32 slot;
> > };
> >
> > /**
> > @@ -382,10 +386,57 @@ static int mt7621_pci_parse_request_of_pci_ranges(struct mt7621_pcie *pcie)
> > return 0;
> > }
> >
> > +static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> > + struct device_node *node,
> > + int slot)
> > +{
> > + struct mt7621_pcie_port *port;
> > + struct device *dev = pcie->dev;
> > + struct device_node *pnode = dev->of_node;
> > + struct resource regs;
> > + char name[6];
> > + int err;
> > +
> > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > + if (!port)
> > + return -ENOMEM;
> > +
> > + err = of_address_to_resource(pnode, slot + 1, ®s);
> > + if (err) {
> > + dev_err(dev, "missing \"reg\" property\n");
> > + return err;
> > + }
> > +
> > + port->base = devm_ioremap_resource(dev, ®s);
> > + if (IS_ERR(port->base))
> > + return PTR_ERR(port->base);
> > +
> > + snprintf(name, sizeof(name), "pcie%d", slot);
> > + port->pcie_clk = devm_clk_get(dev, name);
> > + if (IS_ERR(port->pcie_clk)) {
> > + dev_err(dev, "failed to get pcie%d clock\n", slot);
> > + return PTR_ERR(port->pcie_clk);
> > + }
>
> This is problematic.
> The clocks are defined in the dtsi as:
>
> clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>;
> clock-names = "pcie0", "pcie1", "pcie2";
> and clkctrl is
> clkctrl: clkctrl {
> compatible = "ralink,rt2880-clock";
> #clock-cells = <1>;
> };
>
> but there is no driver that declares compatibility with
> ralink,rt2880-clock.
> So devm_clk_get() cannot find any clocks, and returns an error.
> I worked around this by just ignoring the error, but then something
> else went wrong in a subsequent patch - haven't worked out what yet.
>
> Probably the *right* way to fix this is to add a driver for
> ralink,rt2880-clock but I don't know what such a driver would do.
>
> In any case, I don't plan to pursue this particular issue further until
> I understand the other things that are happening with this patch set.
Thanks for your time Neil in testing this.
If getting the clocks fail then we cannot get enable any port so I suspect the
next thing that gets wrong should be in 'mt7621_pcie_enable_port' when we try to
enable the clock for the port with the call to 'clk_prepare_enable'.
I though the ralink clock driver exists because of its references in
the dtsi file but
I didn't check for a compatible string in the sources. So I think is
my bad here.
Let me know any other thing you want to understand from this patchset
but I think the
actual code should be a clean way to go with this driver (maybe
separating also the phy
part to a phy-pci driver to get all cleaner). Let me know your opinion in this.
>
> Thanks,
> NeilBrown
Best regards,
Sergio Paracuellos
>
>
>
> > +
> > + port->pcie_rst = devm_reset_control_get_exclusive(dev, name);
> > + if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
> > + dev_err(dev, "failed to get pcie%d reset control\n", slot);
> > + return PTR_ERR(port->pcie_rst);
> > + }
> > +
> > + port->slot = slot;
> > + port->pcie = pcie;
> > +
> > + INIT_LIST_HEAD(&port->list);
> > + list_add_tail(&port->list, &pcie->ports);
> > +
> > + return 0;
> > +}
> > +
> > static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
> > {
> > struct device *dev = pcie->dev;
> > - struct device_node *node = dev->of_node;
> > + struct device_node *node = dev->of_node, *child;
> > struct resource regs;
> > int err;
> >
> > @@ -399,6 +450,22 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
> > if (IS_ERR(pcie->base))
> > return PTR_ERR(pcie->base);
> >
> > + for_each_available_child_of_node(node, child) {
> > + int slot;
> > +
> > + err = of_pci_get_devfn(child);
> > + if (err < 0) {
> > + dev_err(dev, "failed to parse devfn: %d\n", err);
> > + return err;
> > + }
> > +
> > + slot = PCI_SLOT(err);
> > +
> > + err = mt7621_pcie_parse_port(pcie, child, slot);
> > + if (err)
> > + return err;
> > + }
> > +
> > return 0;
> > }
> >
> > --
> > 2.7.4
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
