| Message ID | 20200803164823.414772-1-olteanv@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: dsa: sja1105: use detected device id instead of DT one on mismatch | expand |
From: Vladimir Oltean <olteanv@gmail.com> Date: Mon, 3 Aug 2020 19:48:23 +0300 > Although we can detect the chip revision 100% at runtime, it is useful > to specify it in the device tree compatible string too, because > otherwise there would be no way to assess the correctness of device tree > bindings statically, without booting a board (only some switch versions > have internal RGMII delays and/or an SGMII port). > > But for testing the P/Q/R/S support, what I have is a reworked board > with the SJA1105T replaced by a pin-compatible SJA1105Q, and I don't > want to keep a separate device tree blob just for this one-off board. > Since just the chip has been replaced, its RGMII delay setup is > inherently the same (meaning: delays added by the PHY on the slave > ports, and by PCB traces on the fixed-link CPU port). > > For this board, I'd rather have the driver shout at me, but go ahead and > use what it found even if it doesn't match what it's been told is there. > > [ 2.970826] sja1105 spi0.1: Device tree specifies chip SJA1105T but found SJA1105Q, please fix it! > [ 2.980010] sja1105 spi0.1: Probed switch chip: SJA1105Q > [ 3.005082] sja1105 spi0.1: Enabled switch tagging > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Andrew/Florian, do we really want to set a precedence for doing this kind of fallback in our drivers? Thanks.
On 8/4/2020 3:59 PM, David Miller wrote: > From: Vladimir Oltean <olteanv@gmail.com> > Date: Mon, 3 Aug 2020 19:48:23 +0300 > >> Although we can detect the chip revision 100% at runtime, it is useful >> to specify it in the device tree compatible string too, because >> otherwise there would be no way to assess the correctness of device tree >> bindings statically, without booting a board (only some switch versions >> have internal RGMII delays and/or an SGMII port). >> >> But for testing the P/Q/R/S support, what I have is a reworked board >> with the SJA1105T replaced by a pin-compatible SJA1105Q, and I don't >> want to keep a separate device tree blob just for this one-off board. >> Since just the chip has been replaced, its RGMII delay setup is >> inherently the same (meaning: delays added by the PHY on the slave >> ports, and by PCB traces on the fixed-link CPU port). >> >> For this board, I'd rather have the driver shout at me, but go ahead and >> use what it found even if it doesn't match what it's been told is there. >> >> [ 2.970826] sja1105 spi0.1: Device tree specifies chip SJA1105T but found SJA1105Q, please fix it! >> [ 2.980010] sja1105 spi0.1: Probed switch chip: SJA1105Q >> [ 3.005082] sja1105 spi0.1: Enabled switch tagging >> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > Andrew/Florian, do we really want to set a precedence for doing this > kind of fallback in our drivers? Not a big fan of it, and the justification is a little bit weak IMHO, especially since one could argue that the boot agent providing the FDT could do that check and present an appropriate compatible string to the kernel. That said, there is nothing obviously wrong about this proposal and at the end of the day, what people care about is that the right model gets picked up, whether that happens solely via compatibility strings or run time detection can be left to the discretion of the driver. Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
From: Vladimir Oltean <olteanv@gmail.com> Date: Mon, 3 Aug 2020 19:48:23 +0300 > Although we can detect the chip revision 100% at runtime, it is useful > to specify it in the device tree compatible string too, because > otherwise there would be no way to assess the correctness of device tree > bindings statically, without booting a board (only some switch versions > have internal RGMII delays and/or an SGMII port). > > But for testing the P/Q/R/S support, what I have is a reworked board > with the SJA1105T replaced by a pin-compatible SJA1105Q, and I don't > want to keep a separate device tree blob just for this one-off board. > Since just the chip has been replaced, its RGMII delay setup is > inherently the same (meaning: delays added by the PHY on the slave > ports, and by PCB traces on the fixed-link CPU port). > > For this board, I'd rather have the driver shout at me, but go ahead and > use what it found even if it doesn't match what it's been told is there. > > [ 2.970826] sja1105 spi0.1: Device tree specifies chip SJA1105T but found SJA1105Q, please fix it! > [ 2.980010] sja1105 spi0.1: Probed switch chip: SJA1105Q > [ 3.005082] sja1105 spi0.1: Enabled switch tagging > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Applied.
On Tue, Aug 04, 2020 at 08:01:46PM -0700, Florian Fainelli wrote: > On 8/4/2020 3:59 PM, David Miller wrote: > > From: Vladimir Oltean <olteanv@gmail.com> > > Date: Mon, 3 Aug 2020 19:48:23 +0300 > > > >> Although we can detect the chip revision 100% at runtime, it is useful > >> to specify it in the device tree compatible string too, because > >> otherwise there would be no way to assess the correctness of device tree > >> bindings statically, without booting a board (only some switch versions > >> have internal RGMII delays and/or an SGMII port). > >> > >> But for testing the P/Q/R/S support, what I have is a reworked board > >> with the SJA1105T replaced by a pin-compatible SJA1105Q, and I don't > >> want to keep a separate device tree blob just for this one-off board. > >> Since just the chip has been replaced, its RGMII delay setup is > >> inherently the same (meaning: delays added by the PHY on the slave > >> ports, and by PCB traces on the fixed-link CPU port). > >> > >> For this board, I'd rather have the driver shout at me, but go ahead and > >> use what it found even if it doesn't match what it's been told is there. > >> > >> [ 2.970826] sja1105 spi0.1: Device tree specifies chip SJA1105T but found SJA1105Q, please fix it! > >> [ 2.980010] sja1105 spi0.1: Probed switch chip: SJA1105Q > >> [ 3.005082] sja1105 spi0.1: Enabled switch tagging > >> > >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > > > Andrew/Florian, do we really want to set a precedence for doing this > > kind of fallback in our drivers? > > Not a big fan of it, and the justification is a little bit weak IMHO, > especially since one could argue that the boot agent providing the FDT > could do that check and present an appropriate compatible string to the > kernel. I'll admit I'm not a huge fan either, but what you're suggesting is basically to move the whole problem one level lower (somebody would still have to be aware about the device id mismatch). I _was_ going to eventually patch the U-Boot driver to adapt to the real device id too, but only for its own use of networking. I am an even smaller fan of having to do a fdt fixup from U-Boot, then I'd have to rely on that always being there to do its job properly. I've been using this board with a local fdt blob for more than one year now, but it's inconvenient for me to have custom tftp commands for this one board only. I hope I'm not setting for a behavior that might be abused, tbh I don't really see how. At the end of the day though, I don't see why the driver would have to be as punishing as to refuse to probe when it can, warning is more than enough. Thanks, -Vladimir
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index 5079e4aeef80..c3f6f124e5f0 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -3391,11 +3391,14 @@ static const struct dsa_switch_ops sja1105_switch_ops = { .devlink_param_set = sja1105_devlink_param_set, }; +static const struct of_device_id sja1105_dt_ids[]; + static int sja1105_check_device_id(struct sja1105_private *priv) { const struct sja1105_regs *regs = priv->info->regs; u8 prod_id[SJA1105_SIZE_DEVICE_ID] = {0}; struct device *dev = &priv->spidev->dev; + const struct of_device_id *match; u32 device_id; u64 part_no; int rc; @@ -3405,12 +3408,6 @@ static int sja1105_check_device_id(struct sja1105_private *priv) if (rc < 0) return rc; - if (device_id != priv->info->device_id) { - dev_err(dev, "Expected device ID 0x%llx but read 0x%x\n", - priv->info->device_id, device_id); - return -ENODEV; - } - rc = sja1105_xfer_buf(priv, SPI_READ, regs->prod_id, prod_id, SJA1105_SIZE_DEVICE_ID); if (rc < 0) @@ -3418,13 +3415,29 @@ static int sja1105_check_device_id(struct sja1105_private *priv) sja1105_unpack(prod_id, &part_no, 19, 4, SJA1105_SIZE_DEVICE_ID); - if (part_no != priv->info->part_no) { - dev_err(dev, "Expected part number 0x%llx but read 0x%llx\n", - priv->info->part_no, part_no); - return -ENODEV; + for (match = sja1105_dt_ids; match->compatible; match++) { + const struct sja1105_info *info = match->data; + + /* Is what's been probed in our match table at all? */ + if (info->device_id != device_id || info->part_no != part_no) + continue; + + /* But is it what's in the device tree? */ + if (priv->info->device_id != device_id || + priv->info->part_no != part_no) { + dev_warn(dev, "Device tree specifies chip %s but found %s, please fix it!\n", + priv->info->name, info->name); + /* It isn't. No problem, pick that up. */ + priv->info = info; + } + + return 0; } - return 0; + dev_err(dev, "Unexpected {device ID, part number}: 0x%x 0x%llx\n", + device_id, part_no); + + return -ENODEV; } static int sja1105_probe(struct spi_device *spi)
Although we can detect the chip revision 100% at runtime, it is useful to specify it in the device tree compatible string too, because otherwise there would be no way to assess the correctness of device tree bindings statically, without booting a board (only some switch versions have internal RGMII delays and/or an SGMII port). But for testing the P/Q/R/S support, what I have is a reworked board with the SJA1105T replaced by a pin-compatible SJA1105Q, and I don't want to keep a separate device tree blob just for this one-off board. Since just the chip has been replaced, its RGMII delay setup is inherently the same (meaning: delays added by the PHY on the slave ports, and by PCB traces on the fixed-link CPU port). For this board, I'd rather have the driver shout at me, but go ahead and use what it found even if it doesn't match what it's been told is there. [ 2.970826] sja1105 spi0.1: Device tree specifies chip SJA1105T but found SJA1105Q, please fix it! [ 2.980010] sja1105 spi0.1: Probed switch chip: SJA1105Q [ 3.005082] sja1105 spi0.1: Enabled switch tagging Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- drivers/net/dsa/sja1105/sja1105_main.c | 35 ++++++++++++++++++-------- 1 file changed, 24 insertions(+), 11 deletions(-)