The 11/17/2021 10:52, Philipp Zabel wrote:
>
> Hi Horatio,
Hi Phillip,
>
> On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote:
> > +static int lan966x_reset_switch(struct lan966x *lan966x)
> > +{
> > + struct reset_control *reset;
> > + int val = 0;
> > + int ret;
> > +
> > + reset = devm_reset_control_get_shared(lan966x->dev, "switch");
> > + if (IS_ERR(reset))
> > + dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n",
> > + PTR_ERR(reset));
> > + else
> > + reset_control_reset(reset);
>
> According to the device tree bindings, both resets are required.
> I'd expect this to return on error.
> Is there any chance of the device working with out the switch reset
> being triggered?
The only case that I see is if the bootloader triggers this switch
reset and then when bootloader starts the kernel and doesn't set back
the switch in reset. Is this a valid scenario or is a bug in the
bootloader?
>
> > +
> > + reset = devm_reset_control_get_shared(lan966x->dev, "phy");
> > + if (IS_ERR(reset)) {
> > + dev_warn(lan966x->dev, "Could not obtain phy reset: %ld\n",
> > + PTR_ERR(reset));
> > + } else {
> > + reset_control_reset(reset);
> > + }
>
> Same as above.
> Consider printing errors with %pe or dev_err_probe().
>
> > + lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan966x, SYS_RESET_CFG);
> > + lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan966x, SYS_RAM_INIT);
> > + ret = readx_poll_timeout(lan966x_ram_init, lan966x,
> > + val, (val & BIT(1)) == 0, READL_SLEEP_US,
> > + READL_TIMEOUT_US);
> > + if (ret)
> > + return ret;
> > +
> > + lan_wr(SYS_RESET_CFG_CORE_ENA_SET(1), lan966x, SYS_RESET_CFG);
> > +
> > + return 0;
> > +}
> > +
> > +static int lan966x_probe(struct platform_device *pdev)
> > +{
> > + struct fwnode_handle *ports, *portnp;
> > + struct lan966x *lan966x;
> > + int err, i;
> > +
> > + lan966x = devm_kzalloc(&pdev->dev, sizeof(*lan966x), GFP_KERNEL);
> > + if (!lan966x)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, lan966x);
> > + lan966x->dev = &pdev->dev;
> > +
> > + ports = device_get_named_child_node(&pdev->dev, "ethernet-ports");
> > + if (!ports) {
> > + dev_err(&pdev->dev, "no ethernet-ports child not found\n");
> > + err = -ENODEV;
> > + goto out;
>
> No need to goto as long as there's just a "return err;" after the out:
> label.
True, I will udate this.
>
> > + }
> > +
> > + err = lan966x_create_targets(pdev, lan966x);
> > + if (err)
> > + goto out;
> > +
> > + if (lan966x_reset_switch(lan966x)) {
> > + err = -EINVAL;
>
> This should propagate the error returned from lan966x_reset_switch()
> instead.
I will fix it in the next version.
>
> > + goto out;
> > + }
> > +
> > + i = 0;
> > + fwnode_for_each_available_child_node(ports, portnp)
> > + ++i;
> > +
> > + lan966x->num_phys_ports = i;
> > + lan966x->ports = devm_kcalloc(&pdev->dev, lan966x->num_phys_ports,
> > + sizeof(struct lan966x_port *),
> > + GFP_KERNEL);
>
> if (!lan966x->ports)
> return -ENOMEM;
Good catch.
>
> regards
> Philipp
--
/Horatiu
