On 07/10/2024 18:36, Markus Stockhausen wrote:
> The Realtek Otto platform is a series of 4 different MIPS32 based network
> switch SoCs. They consist of:
>
> - RTL838x: 500MHz single core, up to 28 ports 20GBps switching capacity
> - RTL839x: 700MHz single core, up to 52 ports 100GBps switching capacity
> - RTL930x: 700MHz single core, up to 28 ports 120GBps switching capacity
> - RTL931x: 1.4GHz dual core, up to 52 ports 170GBps switching capacity
>
...
> +
> +static int rtsds_phy_power_on(struct phy *phy)
> +{
> + struct rtsds_macro *macro = phy_get_drvdata(phy);
> + struct rtsds_ctrl *ctrl = macro->ctrl;
> + u32 sid = macro->sid;
> + int ret;
> +
> + if (rtsds_readonly(ctrl, sid))
> + return 0;
> +
> + mutex_lock(&ctrl->lock);
> +// ret = rtsds_run_event(ctrl, sid, RTSDS_EVENT_POWER_ON);
Dead code? What is the point of this?
> + mutex_unlock(&ctrl->lock);
> +
> + if (ret)
> + dev_err(ctrl->dev, "power on failed for SerDes %d\n", sid);
> +
> + return ret;
> +}
> +
...
> +
> +static ssize_t rtsds_dbg_mode_write(struct file *file, const char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct seq_file *seqf = file->private_data;
> + struct rtsds_macro *macro = dev_get_drvdata(seqf->private);
> + struct rtsds_ctrl *ctrl = macro->ctrl;
> + int ret, hwmode, phymode, sid = macro->sid;
> +
> + ret = kstrtou32_from_user(userbuf, count, 16, &hwmode);
> + if (ret)
> + return ret;
> + /*
> + * Allow to set arbitrary modes into the SerDes to improve error analysis. Accept that
> + * this might confuse the internal state tracking.
> + */
> + phymode = rtsds_hwmode_to_phymode(ctrl, hwmode);
> + rtsds_phy_set_mode_int(ctrl, sid, phymode, hwmode);
Interfasce which confuses internal state is a bad interface. Drop.
> +
> + return count;
> +}
> +DEFINE_SHOW_STORE_ATTRIBUTE(rtsds_dbg_mode);
> +
> +static ssize_t rtsds_dbg_reset_show(struct seq_file *seqf, void *unused)
> +{
> + return 0;
> +}
> +
> +static ssize_t rtsds_dbg_reset_write(struct file *file, const char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct seq_file *seqf = file->private_data;
> + struct rtsds_macro *macro = dev_get_drvdata(seqf->private);
> + struct rtsds_ctrl *ctrl = macro->ctrl;
> + int ret, reset, sid = macro->sid;
> +
> + ret = kstrtou32_from_user(userbuf, count, 10, &reset);
> + if (ret || reset != 1)
> + return ret;
> +
> + rtsds_phy_reset_int(ctrl, sid);
> +
> + return count;
> +}
> +DEFINE_SHOW_STORE_ATTRIBUTE(rtsds_dbg_reset);
That's not a debugfs interface. Drop reset.
> +
> +static int rtsds_dbg_registers_show(struct seq_file *seqf, void *unused)
> +{
> + struct rtsds_macro *macro = dev_get_drvdata(seqf->private);
> + struct rtsds_ctrl *ctrl = macro->ctrl;
> + u32 page = 0, reg, sid = macro->sid;
> +
> + seq_printf(seqf, "%*s", 12, "");
> + for (int i = 0; i < 32; i++)
> + seq_printf(seqf, " %02X", i);
> +
> + while (page < ctrl->conf->page_cnt) {
> + if (page < RTSDS_PAGE_NAMES && rtsds_page_name[page])
> + seq_printf(seqf, "\n%*s: ", -11, rtsds_page_name[page]);
> + else if (page == 64 || page == 128)
> + seq_printf(seqf, "\n\nXGMII_%d : ", page >> 8);
> + else
> + seq_printf(seqf, "\nPAGE_%03d : ", page);
> + for (reg = 0; reg < 32; reg++)
> + seq_printf(seqf, "%04X ", ctrl->conf->read(ctrl, sid, page, reg));
> +
> + page++;
> + }
> + seq_puts(seqf, "\n");
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rtsds_dbg_registers);
> +
> +static int rtsds_dbg_polarity_show(struct seq_file *seqf, void *unused)
> +{
> + struct rtsds_macro *macro = dev_get_drvdata(seqf->private);
> + struct rtsds_ctrl *ctrl = macro->ctrl;
> + u32 reg, sid = macro->sid;
> +
> + reg = ctrl->conf->read(ctrl, sid, RTSDS_PAGE_SDS, 0);
> +
> + seq_puts(seqf, "tx polarity: ");
> + seq_puts(seqf, reg & RTSDS_BITS_INV_HSO ? "inverse" : "normal");
> + seq_puts(seqf, "\nrx polarity: ");
> + seq_puts(seqf, reg & RTSDS_BITS_INV_HSI ? "inverse" : "normal");
> + seq_puts(seqf, "\n");
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rtsds_dbg_polarity);
> +
> +static void rtsds_dbg_init(struct rtsds_ctrl *ctrl, u32 sid)
> +{
> + debugfs_create_file("mode", 0600, ctrl->sds[sid].phy->debugfs,
> + &ctrl->sds[sid].phy->dev, &rtsds_dbg_mode_fops);
> +
> + debugfs_create_file("reset", 0200, ctrl->sds[sid].phy->debugfs,
> + &ctrl->sds[sid].phy->dev, &rtsds_dbg_reset_fops);
> +
> + debugfs_create_file("polarity", 0400, ctrl->sds[sid].phy->debugfs,
> + &ctrl->sds[sid].phy->dev, &rtsds_dbg_polarity_fops);
> +
> + debugfs_create_file("registers", 0400, ctrl->sds[sid].phy->debugfs,
> + &ctrl->sds[sid].phy->dev, &rtsds_dbg_registers_fops);
> +}
> +#endif /* CONFIG_DEBUG_FS */
> +
> +
> +static int rtsds_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct phy_provider *provider;
> + struct rtsds_ctrl *ctrl;
> + int ret;
> +
> + if (!np)
> + return -EINVAL;
How is this possible? Drop.
> +
> + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> + if (!ctrl)
> + return -ENOMEM;
> +
> + ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(ctrl->base)) {
> + dev_err(dev, "failed to map SerDes memory\n");
> + return PTR_ERR(ctrl->base);
> + }
> +
> + ctrl->dev = dev;
> + ctrl->conf = (struct rtsds_conf *)of_device_get_match_data(dev);
> +
> + ret = of_property_read_u32(np, "realtek,controlled-ports", &ctrl->sds_mask);
> + if (ret)
> + ctrl->sds_mask = GENMASK(ctrl->conf->sds_cnt, 0);
> +
> + for (u32 sid = 0; sid < ctrl->conf->sds_cnt; sid++) {
> + ret = rtsds_phy_create(ctrl, sid);
> + if (ret) {
> + dev_err(dev, "failed to create PHY for SerDes %d\n", sid);
> + return ret;
> + }
> + }
> +
> + mutex_init(&ctrl->lock);
> + dev_set_drvdata(dev, ctrl);
> + provider = devm_of_phy_provider_register(dev, rtsds_simple_xlate);
> + rtsds_setup(ctrl);
> + dev_info(dev, "initialized (%d SerDes, %d pages, 32 registers, mask 0x%04x)",
> + ctrl->conf->sds_cnt, ctrl->conf->page_cnt, ctrl->sds_mask);
> +
> + return PTR_ERR_OR_ZERO(provider);
> +}
> +
...
> +static const struct of_device_id rtsds_compatible_ids[] = {
> + { .compatible = "realtek,rtl8380-serdes",
> + .data = &rtsds_838x_conf,
> + },
> + { .compatible = "realtek,rtl8390-serdes",
> + .data = &rtsds_839x_conf,
> + },
> + { .compatible = "realtek,rtl9300-serdes",
> + .data = &rtsds_930x_conf,
> + },
> + { .compatible = "realtek,rtl9310-serdes",
> + .data = &rtsds_931x_conf,
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, rtsds_compatible_ids);
> +
> +static struct platform_driver rtsds_platform_driver = {
> + .probe = rtsds_probe,
> + .driver = {
> + .name = "realtek,otto-serdes",
> + .of_match_table = of_match_ptr(rtsds_compatible_ids),
Drop of_match_ptr, you have warning because of it.
> + },
> +};
> +
> +module_platform_driver(rtsds_platform_driver);
> +
> +MODULE_AUTHOR("Markus Stockhausen <markus.stockhausen@xxxxxx>");
> +MODULE_DESCRIPTION("SerDes driver for Realtek RTL83xx, RTL93xx switch SoCs");
> +MODULE_LICENSE("GPL");
...
> +
> +struct rtsds_macro {
> + struct rtsds_ctrl *ctrl;
> + u32 sid;
> +};
> +
> +struct rtsds_conf {
> + u32 sds_cnt;
> + u32 page_cnt;
> + int (*read)(struct rtsds_ctrl *ctrl, u32 idx, u32 page, u32 reg);
> + int (*mask)(struct rtsds_ctrl *ctrl, u32 idx, u32 page, u32 reg, u32 val, u32 mask);
> + int (*reset)(struct rtsds_ctrl *ctrl, u32 idx);
> + int (*set_mode)(struct rtsds_ctrl *ctrl, u32 idx, int mode);
> + int (*get_mode)(struct rtsds_ctrl *ctrl, u32 idx);
> + int mode_map[PHY_INTERFACE_MODE_MAX];
> + struct rtsds_seq *sequence[RTSDS_EVENT_CNT];
> +};
> +
> +/*
> + * This SerDes module should be written in quite a clean way so that direct calls are
> + * not needed. The following functions are provided just in case ...
> + */
Drop code "just in case".
Best regards,
Krzysztof
