[PATCH v2 0/3] can: mcp251xfd: PLL support

[PATCH v2 0/3] can: mcp251xfd: PLL support

[Marc Kleine-Budde]

Hello,

I've looked at Magnus Aagaard Sørensen's patches and decided the PLL idea looks
quite good, but the runtime pm change should better made after adding PLL support.

This series focuses on adding PLL support only. Consider this as an RFC, the
patches need proper description.

I've tested the setup here with two mcp2518fd, where one supplies the 4MHz for
the other and it works for me.

Testing welcome.

regards,
Marc

[PATCH v2 1/3] can: mcp251xfd: mcp251xfd_chip_wake(): renamed from mcp251xfd_chip_clock_enable()

[Marc Kleine-Budde]

Co-developed-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
Not-Singed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index c3f49543ff26..c36f5f14d50c 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -519,13 +519,16 @@ static inline bool mcp251xfd_osc_invalid(u32 reg)
 	return reg == 0x0 || reg == 0xffffffff;
 }
 
-static int mcp251xfd_chip_clock_enable(const struct mcp251xfd_priv *priv)
+static int mcp251xfd_chip_wake(const struct mcp251xfd_priv *priv)
 {
 	u32 osc, osc_reference, osc_mask;
 	int err;
 
-	/* Set Power On Defaults for "Clock Output Divisor" and remove
-	 * "Oscillator Disable" bit.
+	/* For normal sleep on MCP2517FD and MCP2518FD, clearing
+	 * "Oscillator Disable" will wake the chip. For low power mode
+	 * on MCP2518FD, asserting the chip select will wake the
+	 * chip. Writing to the Oscillator register will wake it in
+	 * both cases.
 	 */
 	osc = FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
 			 MCP251XFD_REG_OSC_CLKODIV_10);
@@ -569,10 +572,10 @@ static int mcp251xfd_chip_softreset_do(const struct mcp251xfd_priv *priv)
 	const __be16 cmd = mcp251xfd_cmd_reset();
 	int err;
 
-	/* The Set Mode and SPI Reset command only seems to works if
-	 * the controller is not in Sleep Mode.
+	/* The Set Mode and SPI Reset command only works if the
+	 * controller is not in Sleep Mode.
 	 */
-	err = mcp251xfd_chip_clock_enable(priv);
+	err = mcp251xfd_chip_wake(priv);
 	if (err)
 		return err;
 
-- 
2.28.0

[PATCH v2 2/3] can: mcp251xfd: mcp251xfd_chip_timestamp_init():

[Marc Kleine-Budde]

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index c36f5f14d50c..b8304b7096d1 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -665,10 +665,11 @@ static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
 	osc = MCP251XFD_REG_OSC_LPMEN |
 		FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
 			   MCP251XFD_REG_OSC_CLKODIV_10);
-	err = regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
-	if (err)
-		return err;
+	return regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
+}
 
+static int mcp251xfd_chip_timestamp_init(const struct mcp251xfd_priv *priv)
+{
 	/* Set Time Base Counter Prescaler to 1.
 	 *
 	 * This means an overflow of the 32 bit Time Base Counter
@@ -1038,6 +1039,10 @@ static int mcp251xfd_chip_start(struct mcp251xfd_priv *priv)
 	if (err)
 		goto out_chip_stop;
 
+	err = mcp251xfd_chip_timestamp_init(priv);
+	if (err)
+		goto out_chip_stop;
+
 	err = mcp251xfd_set_bittiming(priv);
 	if (err)
 		goto out_chip_stop;
-- 
2.28.0

[PATCH v2 3/3] can: mcp251xfd: add support for internal PLL

[Marc Kleine-Budde]

The PLL is enabled if the configured clock is less than or equal to 10 times
the max clock frequency.

The device will operate with two different SPI speeds. A slow speed determined
by the clock without the PLL enabled, and a fast speed derived from the
frequency with the PLL enabled.

Link: https://lore.kernel.org/r/20201015124401.2766-3-mas@csselectronics.com
Co-developed-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
Not-Singed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 73 +++++++++++++++----
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |  3 +
 2 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index b8304b7096d1..2be643c6e243 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -479,8 +479,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
 	if (err)
 		return err;
 
-	if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP || nowait)
+	if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP || nowait) {
+		if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP)
+			priv->spi->max_speed_hz = priv->spi_max_speed_hz_slow;
+
 		return 0;
+	}
 
 	err = regmap_read_poll_timeout(priv->map_reg, MCP251XFD_REG_CON, con,
 				       FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
@@ -655,7 +659,7 @@ static int mcp251xfd_chip_softreset(const struct mcp251xfd_priv *priv)
 
 static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
 {
-	u32 osc;
+	u32 osc, osc_reference, osc_mask;
 	int err;
 
 	/* Activate Low Power Mode on Oscillator Disable. This only
@@ -665,7 +669,34 @@ static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
 	osc = MCP251XFD_REG_OSC_LPMEN |
 		FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
 			   MCP251XFD_REG_OSC_CLKODIV_10);
-	return regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
+	osc_reference = MCP251XFD_REG_OSC_OSCRDY;
+	osc_mask = MCP251XFD_REG_OSC_OSCRDY | MCP251XFD_REG_OSC_PLLRDY;
+
+	if (priv->pll_enable) {
+		osc |= MCP251XFD_REG_OSC_PLLEN;
+		osc_reference |= MCP251XFD_REG_OSC_PLLRDY;
+	}
+
+	err = regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
+	if (err)
+		return err;
+
+	/* Wait for "Oscillator + PLL Ready" bit */
+	err = regmap_read_poll_timeout(priv->map_reg, MCP251XFD_REG_OSC, osc,
+				       (osc & osc_mask) == osc_reference,
+				       MCP251XFD_OSC_STAB_SLEEP_US,
+				       MCP251XFD_OSC_STAB_TIMEOUT_US);
+
+	if (err == -ETIMEDOUT) {
+		netdev_err(priv->ndev,
+			   "Timeout waiting for Oscillator + PLL Ready (osc=0x%08x, osc_reference=0x%08x)\n",
+			   osc, osc_reference);
+		return -ETIMEDOUT;
+	}
+
+	priv->spi->max_speed_hz = priv->spi_max_speed_hz_fast;
+
+	return 0;
 }
 
 static int mcp251xfd_chip_timestamp_init(const struct mcp251xfd_priv *priv)
@@ -2604,11 +2635,12 @@ mcp251xfd_register_done(const struct mcp251xfd_priv *priv)
 		return err;
 
 	netdev_info(priv->ndev,
-		    "%s rev%lu.%lu (%cRX_INT %cMAB_NO_WARN %cCRC_REG %cCRC_RX %cCRC_TX %cECC %cHD c:%u.%02uMHz m:%u.%02uMHz r:%u.%02uMHz e:%u.%02uMHz) successfully initialized.\n",
+		    "%s rev%lu.%lu (%cRX_INT %cPLL %cMAB_NO_WARN %cCRC_REG %cCRC_RX %cCRC_TX %cECC %cHD c:%u.%02uMHz m:%u.%02uMHz rs:%u.%02uMHz rf:%u.%02uMHz ef:%u.%02uMHz) successfully initialized.\n",
 		    mcp251xfd_get_model_str(priv),
 		    FIELD_GET(MCP251XFD_REG_DEVID_ID_MASK, dev_id),
 		    FIELD_GET(MCP251XFD_REG_DEVID_REV_MASK, dev_id),
 		    priv->rx_int ? '+' : '-',
+		    priv->pll_enable ? '+' : '-',
 		    MCP251XFD_QUIRK_ACTIVE(MAB_NO_WARN),
 		    MCP251XFD_QUIRK_ACTIVE(CRC_REG),
 		    MCP251XFD_QUIRK_ACTIVE(CRC_RX),
@@ -2619,8 +2651,10 @@ mcp251xfd_register_done(const struct mcp251xfd_priv *priv)
 		    priv->can.clock.freq % 1000000 / 1000 / 10,
 		    priv->spi_max_speed_hz_orig / 1000000,
 		    priv->spi_max_speed_hz_orig % 1000000 / 1000 / 10,
-		    priv->spi->max_speed_hz / 1000000,
-		    priv->spi->max_speed_hz % 1000000 / 1000 / 10,
+		    priv->spi_max_speed_hz_slow / 1000000,
+		    priv->spi_max_speed_hz_slow % 1000000 / 1000 / 10,
+		    priv->spi_max_speed_hz_fast / 1000000,
+		    priv->spi_max_speed_hz_fast % 1000000 / 1000 / 10,
 		    effective_speed_hz / 1000000,
 		    effective_speed_hz % 1000000 / 1000 / 10);
 
@@ -2650,6 +2684,10 @@ static int mcp251xfd_register(struct mcp251xfd_priv *priv)
 	if (err)
 		goto out_chip_set_mode_sleep;
 
+	err = mcp251xfd_chip_clock_init(priv);
+	if (err)
+		goto out_chip_set_mode_sleep;
+
 	err = mcp251xfd_register_chip_detect(priv);
 	if (err)
 		goto out_chip_set_mode_sleep;
@@ -2743,6 +2781,7 @@ static int mcp251xfd_probe(struct spi_device *spi)
 	struct gpio_desc *rx_int;
 	struct regulator *reg_vdd, *reg_xceiver;
 	struct clk *clk;
+	bool pll_enable = false;
 	u32 freq;
 	int err;
 
@@ -2785,12 +2824,8 @@ static int mcp251xfd_probe(struct spi_device *spi)
 		return -ERANGE;
 	}
 
-	if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER) {
-		dev_err(&spi->dev,
-			"Oscillator frequency (%u Hz) is too low and PLL is not supported.\n",
-			freq);
-		return -ERANGE;
-	}
+	if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER)
+		pll_enable = true;
 
 	ndev = alloc_candev(sizeof(struct mcp251xfd_priv),
 			    MCP251XFD_TX_OBJ_NUM_MAX);
@@ -2806,6 +2841,8 @@ static int mcp251xfd_probe(struct spi_device *spi)
 	priv = netdev_priv(ndev);
 	spi_set_drvdata(spi, priv);
 	priv->can.clock.freq = freq;
+	if (pll_enable)
+		priv->can.clock.freq *= MCP251XFD_OSC_PLL_MULTIPLIER;
 	priv->can.do_set_mode = mcp251xfd_set_mode;
 	priv->can.do_get_berr_counter = mcp251xfd_get_berr_counter;
 	priv->can.bittiming_const = &mcp251xfd_bittiming_const;
@@ -2817,6 +2854,7 @@ static int mcp251xfd_probe(struct spi_device *spi)
 	priv->spi = spi;
 	priv->rx_int = rx_int;
 	priv->clk = clk;
+	priv->pll_enable = pll_enable;
 	priv->reg_vdd = reg_vdd;
 	priv->reg_xceiver = reg_xceiver;
 
@@ -2856,7 +2894,16 @@ static int mcp251xfd_probe(struct spi_device *spi)
 	 *
 	 */
 	priv->spi_max_speed_hz_orig = spi->max_speed_hz;
-	spi->max_speed_hz = min(spi->max_speed_hz, freq / 2 / 1000 * 850);
+	priv->spi_max_speed_hz_slow = min(spi->max_speed_hz,
+					  freq / 2 / 1000 * 850);
+	if (priv->pll_enable)
+		priv->spi_max_speed_hz_fast = min(spi->max_speed_hz,
+						  freq *
+						  MCP251XFD_OSC_PLL_MULTIPLIER /
+						  2 / 1000 * 850);
+	else
+		priv->spi_max_speed_hz_fast = priv->spi_max_speed_hz_slow;
+	spi->max_speed_hz = priv->spi_max_speed_hz_slow;
 	spi->bits_per_word = 8;
 	spi->rt = true;
 	err = spi_setup(spi);
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index fa1246e39980..813a791234de 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -579,6 +579,8 @@ struct mcp251xfd_priv {
 
 	struct spi_device *spi;
 	u32 spi_max_speed_hz_orig;
+	u32 spi_max_speed_hz_fast;
+	u32 spi_max_speed_hz_slow;
 
 	struct mcp251xfd_tef_ring tef;
 	struct mcp251xfd_tx_ring tx[1];
@@ -591,6 +593,7 @@ struct mcp251xfd_priv {
 
 	struct gpio_desc *rx_int;
 	struct clk *clk;
+	bool pll_enable;
 	struct regulator *reg_vdd;
 	struct regulator *reg_xceiver;
 
-- 
2.28.0

Re: [PATCH v2 0/3] can: mcp251xfd: PLL support

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1001 bytes --]

On 10/16/20 10:52 PM, Marc Kleine-Budde wrote:
> Hello,
> 
> I've looked at Magnus Aagaard Sørensen's patches and decided the PLL idea looks
> quite good, but the runtime pm change should better made after adding PLL support.
> 
> This series focuses on adding PLL support only. Consider this as an RFC, the
> patches need proper description.
> 
> I've tested the setup here with two mcp2518fd, where one supplies the 4MHz for
> the other and it works for me.
> 
> Testing welcome.

Magnus, I've credited you as the Co-devloper of the patches 1 and 3, can I have
your Signed-off-by? See

https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L423

for details.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] can: mcp251xfd: mcp251xfd_chip_wake(): renamed from mcp251xfd_chip_clock_enable()

[Magnus Aagaard Sørensen]

Signed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>

On 16-10-2020 22:52, Marc Kleine-Budde wrote:
> Co-developed-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
> Not-Singed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index c3f49543ff26..c36f5f14d50c 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -519,13 +519,16 @@ static inline bool mcp251xfd_osc_invalid(u32 reg)
>   	return reg == 0x0 || reg == 0xffffffff;
>   }
>   
> -static int mcp251xfd_chip_clock_enable(const struct mcp251xfd_priv *priv)
> +static int mcp251xfd_chip_wake(const struct mcp251xfd_priv *priv)
>   {
>   	u32 osc, osc_reference, osc_mask;
>   	int err;
>   
> -	/* Set Power On Defaults for "Clock Output Divisor" and remove
> -	 * "Oscillator Disable" bit.
> +	/* For normal sleep on MCP2517FD and MCP2518FD, clearing
> +	 * "Oscillator Disable" will wake the chip. For low power mode
> +	 * on MCP2518FD, asserting the chip select will wake the
> +	 * chip. Writing to the Oscillator register will wake it in
> +	 * both cases.
>   	 */
>   	osc = FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
>   			 MCP251XFD_REG_OSC_CLKODIV_10);
> @@ -569,10 +572,10 @@ static int mcp251xfd_chip_softreset_do(const struct mcp251xfd_priv *priv)
>   	const __be16 cmd = mcp251xfd_cmd_reset();
>   	int err;
>   
> -	/* The Set Mode and SPI Reset command only seems to works if
> -	 * the controller is not in Sleep Mode.
> +	/* The Set Mode and SPI Reset command only works if the
> +	 * controller is not in Sleep Mode.
>   	 */
> -	err = mcp251xfd_chip_clock_enable(priv);
> +	err = mcp251xfd_chip_wake(priv);
>   	if (err)
>   		return err;
>   

Re: [PATCH v2 3/3] can: mcp251xfd: add support for internal PLL

[Magnus Aagaard Sørensen]

Signed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>

On 16-10-2020 22:52, Marc Kleine-Budde wrote:
> The PLL is enabled if the configured clock is less than or equal to 10 times
> the max clock frequency.
>
> The device will operate with two different SPI speeds. A slow speed determined
> by the clock without the PLL enabled, and a fast speed derived from the
> frequency with the PLL enabled.
>
> Link: https://lore.kernel.org/r/20201015124401.2766-3-mas@csselectronics.com
> Co-developed-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
> Not-Singed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 73 +++++++++++++++----
>   drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |  3 +
>   2 files changed, 63 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index b8304b7096d1..2be643c6e243 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -479,8 +479,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
>   	if (err)
>   		return err;
>   
> -	if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP || nowait)
> +	if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP || nowait) {
> +		if (mode_req == MCP251XFD_REG_CON_MODE_SLEEP)
> +			priv->spi->max_speed_hz = priv->spi_max_speed_hz_slow;
> +
>   		return 0;
> +	}
>   
>   	err = regmap_read_poll_timeout(priv->map_reg, MCP251XFD_REG_CON, con,
>   				       FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
> @@ -655,7 +659,7 @@ static int mcp251xfd_chip_softreset(const struct mcp251xfd_priv *priv)
>   
>   static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
>   {
> -	u32 osc;
> +	u32 osc, osc_reference, osc_mask;
>   	int err;
>   
>   	/* Activate Low Power Mode on Oscillator Disable. This only
> @@ -665,7 +669,34 @@ static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
>   	osc = MCP251XFD_REG_OSC_LPMEN |
>   		FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
>   			   MCP251XFD_REG_OSC_CLKODIV_10);
> -	return regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
> +	osc_reference = MCP251XFD_REG_OSC_OSCRDY;
> +	osc_mask = MCP251XFD_REG_OSC_OSCRDY | MCP251XFD_REG_OSC_PLLRDY;
> +
> +	if (priv->pll_enable) {
> +		osc |= MCP251XFD_REG_OSC_PLLEN;
> +		osc_reference |= MCP251XFD_REG_OSC_PLLRDY;
> +	}
> +
> +	err = regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
> +	if (err)
> +		return err;
> +
> +	/* Wait for "Oscillator + PLL Ready" bit */
> +	err = regmap_read_poll_timeout(priv->map_reg, MCP251XFD_REG_OSC, osc,
> +				       (osc & osc_mask) == osc_reference,
> +				       MCP251XFD_OSC_STAB_SLEEP_US,
> +				       MCP251XFD_OSC_STAB_TIMEOUT_US);
> +
> +	if (err == -ETIMEDOUT) {
> +		netdev_err(priv->ndev,
> +			   "Timeout waiting for Oscillator + PLL Ready (osc=0x%08x, osc_reference=0x%08x)\n",
> +			   osc, osc_reference);
> +		return -ETIMEDOUT;
> +	}
> +
> +	priv->spi->max_speed_hz = priv->spi_max_speed_hz_fast;
> +
> +	return 0;
>   }
>   
>   static int mcp251xfd_chip_timestamp_init(const struct mcp251xfd_priv *priv)
> @@ -2604,11 +2635,12 @@ mcp251xfd_register_done(const struct mcp251xfd_priv *priv)
>   		return err;
>   
>   	netdev_info(priv->ndev,
> -		    "%s rev%lu.%lu (%cRX_INT %cMAB_NO_WARN %cCRC_REG %cCRC_RX %cCRC_TX %cECC %cHD c:%u.%02uMHz m:%u.%02uMHz r:%u.%02uMHz e:%u.%02uMHz) successfully initialized.\n",
> +		    "%s rev%lu.%lu (%cRX_INT %cPLL %cMAB_NO_WARN %cCRC_REG %cCRC_RX %cCRC_TX %cECC %cHD c:%u.%02uMHz m:%u.%02uMHz rs:%u.%02uMHz rf:%u.%02uMHz ef:%u.%02uMHz) successfully initialized.\n",
>   		    mcp251xfd_get_model_str(priv),
>   		    FIELD_GET(MCP251XFD_REG_DEVID_ID_MASK, dev_id),
>   		    FIELD_GET(MCP251XFD_REG_DEVID_REV_MASK, dev_id),
>   		    priv->rx_int ? '+' : '-',
> +		    priv->pll_enable ? '+' : '-',
>   		    MCP251XFD_QUIRK_ACTIVE(MAB_NO_WARN),
>   		    MCP251XFD_QUIRK_ACTIVE(CRC_REG),
>   		    MCP251XFD_QUIRK_ACTIVE(CRC_RX),
> @@ -2619,8 +2651,10 @@ mcp251xfd_register_done(const struct mcp251xfd_priv *priv)
>   		    priv->can.clock.freq % 1000000 / 1000 / 10,
>   		    priv->spi_max_speed_hz_orig / 1000000,
>   		    priv->spi_max_speed_hz_orig % 1000000 / 1000 / 10,
> -		    priv->spi->max_speed_hz / 1000000,
> -		    priv->spi->max_speed_hz % 1000000 / 1000 / 10,
> +		    priv->spi_max_speed_hz_slow / 1000000,
> +		    priv->spi_max_speed_hz_slow % 1000000 / 1000 / 10,
> +		    priv->spi_max_speed_hz_fast / 1000000,
> +		    priv->spi_max_speed_hz_fast % 1000000 / 1000 / 10,
>   		    effective_speed_hz / 1000000,
>   		    effective_speed_hz % 1000000 / 1000 / 10);
>   
> @@ -2650,6 +2684,10 @@ static int mcp251xfd_register(struct mcp251xfd_priv *priv)
>   	if (err)
>   		goto out_chip_set_mode_sleep;
>   
> +	err = mcp251xfd_chip_clock_init(priv);
> +	if (err)
> +		goto out_chip_set_mode_sleep;
> +
>   	err = mcp251xfd_register_chip_detect(priv);
>   	if (err)
>   		goto out_chip_set_mode_sleep;
> @@ -2743,6 +2781,7 @@ static int mcp251xfd_probe(struct spi_device *spi)
>   	struct gpio_desc *rx_int;
>   	struct regulator *reg_vdd, *reg_xceiver;
>   	struct clk *clk;
> +	bool pll_enable = false;
>   	u32 freq;
>   	int err;
>   
> @@ -2785,12 +2824,8 @@ static int mcp251xfd_probe(struct spi_device *spi)
>   		return -ERANGE;
>   	}
>   
> -	if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER) {
> -		dev_err(&spi->dev,
> -			"Oscillator frequency (%u Hz) is too low and PLL is not supported.\n",
> -			freq);
> -		return -ERANGE;
> -	}
> +	if (freq <= MCP251XFD_SYSCLOCK_HZ_MAX / MCP251XFD_OSC_PLL_MULTIPLIER)
> +		pll_enable = true;
>   
>   	ndev = alloc_candev(sizeof(struct mcp251xfd_priv),
>   			    MCP251XFD_TX_OBJ_NUM_MAX);
> @@ -2806,6 +2841,8 @@ static int mcp251xfd_probe(struct spi_device *spi)
>   	priv = netdev_priv(ndev);
>   	spi_set_drvdata(spi, priv);
>   	priv->can.clock.freq = freq;
> +	if (pll_enable)
> +		priv->can.clock.freq *= MCP251XFD_OSC_PLL_MULTIPLIER;
>   	priv->can.do_set_mode = mcp251xfd_set_mode;
>   	priv->can.do_get_berr_counter = mcp251xfd_get_berr_counter;
>   	priv->can.bittiming_const = &mcp251xfd_bittiming_const;
> @@ -2817,6 +2854,7 @@ static int mcp251xfd_probe(struct spi_device *spi)
>   	priv->spi = spi;
>   	priv->rx_int = rx_int;
>   	priv->clk = clk;
> +	priv->pll_enable = pll_enable;
>   	priv->reg_vdd = reg_vdd;
>   	priv->reg_xceiver = reg_xceiver;
>   
> @@ -2856,7 +2894,16 @@ static int mcp251xfd_probe(struct spi_device *spi)
>   	 *
>   	 */
>   	priv->spi_max_speed_hz_orig = spi->max_speed_hz;
> -	spi->max_speed_hz = min(spi->max_speed_hz, freq / 2 / 1000 * 850);
> +	priv->spi_max_speed_hz_slow = min(spi->max_speed_hz,
> +					  freq / 2 / 1000 * 850);
> +	if (priv->pll_enable)
> +		priv->spi_max_speed_hz_fast = min(spi->max_speed_hz,
> +						  freq *
> +						  MCP251XFD_OSC_PLL_MULTIPLIER /
> +						  2 / 1000 * 850);
> +	else
> +		priv->spi_max_speed_hz_fast = priv->spi_max_speed_hz_slow;
> +	spi->max_speed_hz = priv->spi_max_speed_hz_slow;
>   	spi->bits_per_word = 8;
>   	spi->rt = true;
>   	err = spi_setup(spi);
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> index fa1246e39980..813a791234de 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> @@ -579,6 +579,8 @@ struct mcp251xfd_priv {
>   
>   	struct spi_device *spi;
>   	u32 spi_max_speed_hz_orig;
> +	u32 spi_max_speed_hz_fast;
> +	u32 spi_max_speed_hz_slow;
>   
>   	struct mcp251xfd_tef_ring tef;
>   	struct mcp251xfd_tx_ring tx[1];
> @@ -591,6 +593,7 @@ struct mcp251xfd_priv {
>   
>   	struct gpio_desc *rx_int;
>   	struct clk *clk;
> +	bool pll_enable;
>   	struct regulator *reg_vdd;
>   	struct regulator *reg_xceiver;
>   

RE: [PATCH v2 1/3] can: mcp251xfd: mcp251xfd_chip_wake(): renamed from mcp251xfd_chip_clock_enable()

[Thomas.Kopp]

> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: Friday, 16 October 2020 22:52
> To: linux-can@vger.kernel.org
> Cc: Magnus Aagaard Sørensen <mas@csselectronics.com>; Marc Kleine-
> Budde <mkl@pengutronix.de>
> Subject: [PATCH v2 1/3] can: mcp251xfd: mcp251xfd_chip_wake():
> renamed from mcp251xfd_chip_clock_enable()
> 
> Co-developed-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
> Not-Singed-off-by: Magnus Aagaard Sørensen <mas@csselectronics.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index c3f49543ff26..c36f5f14d50c 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -519,13 +519,16 @@ static inline bool mcp251xfd_osc_invalid(u32 reg)
>         return reg == 0x0 || reg == 0xffffffff;
>  }
> 
> -static int mcp251xfd_chip_clock_enable(const struct mcp251xfd_priv
> *priv)
> +static int mcp251xfd_chip_wake(const struct mcp251xfd_priv *priv)
>  {
>         u32 osc, osc_reference, osc_mask;
>         int err;
> 
> -       /* Set Power On Defaults for "Clock Output Divisor" and remove
> -        * "Oscillator Disable" bit.
> +       /* For normal sleep on MCP2517FD and MCP2518FD, clearing
> +        * "Oscillator Disable" will wake the chip. For low power mode
> +        * on MCP2518FD, asserting the chip select will wake the
> +        * chip. Writing to the Oscillator register will wake it in
> +        * both cases.
>          */
>         osc = FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
>                          MCP251XFD_REG_OSC_CLKODIV_10);
> @@ -569,10 +572,10 @@ static int mcp251xfd_chip_softreset_do(const
> struct mcp251xfd_priv *priv)
>         const __be16 cmd = mcp251xfd_cmd_reset();
>         int err;
> 
> -       /* The Set Mode and SPI Reset command only seems to works if
> -        * the controller is not in Sleep Mode.
> +       /* The Set Mode and SPI Reset command only works if the
> +        * controller is not in Sleep Mode.
>          */
> -       err = mcp251xfd_chip_clock_enable(priv);
> +       err = mcp251xfd_chip_wake(priv);
>         if (err)
>                 return err;
> 
> --
> 2.28.0

This patch series works fine on my setup (RPI4, MCP2518FD, external 4 MHz crystal). Going through the code I noticed that the Min specified OSC frequency is specified from 1 MHz to 40 MHz. Technically the DS only specifies 4,20 and 40MHz as crystal/resonator options and 2MHz input as the minimum external clock. 4 with PLL and 40 direct are the preferred options for CAN-FD. I think the code is fine, given that the default is 40 MHz and it's the user's responsibility to design in the part according to the DS. We could narrow down the pll clockcheck to only allow 4 MHz though.

All 3 patches in this series:
Tested-by: Thomas Kopp <thomas.kopp@microchip.com>

Best Regards,
Thomas

Re: [PATCH v2 1/3] can: mcp251xfd: mcp251xfd_chip_wake(): renamed from mcp251xfd_chip_clock_enable()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1110 bytes --]

On 10/26/20 10:50 AM, Thomas.Kopp@microchip.com wrote:
> This patch series works fine on my setup (RPI4, MCP2518FD, external 4 MHz
> crystal).

\o/ Thanks \o/

> Going through the code I noticed that the Min specified OSC frequency is
> specified from 1 MHz to 40 MHz. Technically the DS only specifies 4,20 and
> 40MHz as crystal/resonator options and 2MHz input as the minimum external
> clock. 4 with PLL and 40 direct are the preferred options for CAN-FD. I think
> the code is fine, given that the default is 40 MHz and it's the user's
> responsibility to design in the part according to the DS. We could narrow
> down the pll clockcheck to only allow 4 MHz though.

I'll address this in my next patch series.

> All 3 patches in this series:
> Tested-by: Thomas Kopp <thomas.kopp@microchip.com>

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]