[PATCH net 0/2] pull-request: can 2022-05-13

[PATCH net 0/2] pull-request: can 2022-05-13

[Marc Kleine-Budde]

Hello Jakub, hello David,

this is a pull request of 2 patches for net/master.

Both patches are by Jarkko Nikula, target the m_can PCI driver
bindings, and fix usage of wrong bit timing constants for the Elkhart
Lake platform.

regards,
Marc

---

The following changes since commit f3f19f939c11925dadd3f4776f99f8c278a7017b:

  Merge tag 'net-5.18-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net (2022-05-12 11:51:45 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-5.18-20220513

for you to fetch changes up to 3ddd9ed84d8954afcdf7afacf8e142198c3803c3:

  can: m_can: remove support for custom bit timing, take #2 (2022-05-13 14:41:40 +0200)

----------------------------------------------------------------
linux-can-fixes-for-5.18-20220513

----------------------------------------------------------------
Jarkko Nikula (2):
      Revert "can: m_can: pci: use custom bit timings for Elkhart Lake"
      can: m_can: remove support for custom bit timing, take #2

 drivers/net/can/m_can/m_can.c     | 24 +++++---------------
 drivers/net/can/m_can/m_can.h     |  3 ---
 drivers/net/can/m_can/m_can_pci.c | 48 ++++-----------------------------------
 3 files changed, 10 insertions(+), 65 deletions(-)

[PATCH net 1/2] Revert "can: m_can: pci: use custom bit timings for Elkhart Lake"

[Marc Kleine-Budde]

From: Jarkko Nikula <jarkko.nikula@linux.intel.com>

This reverts commit 0e8ffdf3b86dfd44b651f91b12fcae76c25c453b.

Commit 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for
Elkhart Lake") broke the test case using bitrate switching.

| ip link set can0 up type can bitrate 500000 dbitrate 4000000 fd on
| ip link set can1 up type can bitrate 500000 dbitrate 4000000 fd on
| candump can0 &
| cangen can1 -I 0x800 -L 64 -e -fb \
|     -D 11223344deadbeef55667788feedf00daabbccdd44332211 -n 1 -v -v

Above commit does everything correctly according to the datasheet.
However datasheet wasn't correct.

I got confirmation from hardware engineers that the actual CAN
hardware on Intel Elkhart Lake is based on M_CAN version v3.2.0.
Datasheet was mirroring values from an another specification which was
based on earlier M_CAN version leading to wrong bit timings.

Therefore revert the commit and switch back to common bit timings.

Fixes: 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for Elkhart Lake")
Link: https://lore.kernel.org/all/20220512124144.536850-1-jarkko.nikula@linux.intel.com
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Reported-by: Chee Hou Ong <chee.houx.ong@intel.com>
Reported-by: Aman Kumar <aman.kumar@intel.com>
Reported-by: Pallavi Kumari <kumari.pallavi@intel.com>
Cc: <stable@vger.kernel.org> # v5.16+
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can_pci.c | 48 +++----------------------------
 1 file changed, 4 insertions(+), 44 deletions(-)

diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index b56a54d6c5a9..8f184a852a0a 100644
--- a/drivers/net/can/m_can/m_can_pci.c
+++ b/drivers/net/can/m_can/m_can_pci.c
@@ -18,14 +18,9 @@
 
 #define M_CAN_PCI_MMIO_BAR		0
 
+#define M_CAN_CLOCK_FREQ_EHL		200000000
 #define CTL_CSR_INT_CTL_OFFSET		0x508
 
-struct m_can_pci_config {
-	const struct can_bittiming_const *bit_timing;
-	const struct can_bittiming_const *data_timing;
-	unsigned int clock_freq;
-};
-
 struct m_can_pci_priv {
 	struct m_can_classdev cdev;
 
@@ -89,40 +84,9 @@ static struct m_can_ops m_can_pci_ops = {
 	.read_fifo = iomap_read_fifo,
 };
 
-static const struct can_bittiming_const m_can_bittiming_const_ehl = {
-	.name = KBUILD_MODNAME,
-	.tseg1_min = 2,		/* Time segment 1 = prop_seg + phase_seg1 */
-	.tseg1_max = 64,
-	.tseg2_min = 1,		/* Time segment 2 = phase_seg2 */
-	.tseg2_max = 128,
-	.sjw_max = 128,
-	.brp_min = 1,
-	.brp_max = 512,
-	.brp_inc = 1,
-};
-
-static const struct can_bittiming_const m_can_data_bittiming_const_ehl = {
-	.name = KBUILD_MODNAME,
-	.tseg1_min = 2,		/* Time segment 1 = prop_seg + phase_seg1 */
-	.tseg1_max = 16,
-	.tseg2_min = 1,		/* Time segment 2 = phase_seg2 */
-	.tseg2_max = 8,
-	.sjw_max = 4,
-	.brp_min = 1,
-	.brp_max = 32,
-	.brp_inc = 1,
-};
-
-static const struct m_can_pci_config m_can_pci_ehl = {
-	.bit_timing = &m_can_bittiming_const_ehl,
-	.data_timing = &m_can_data_bittiming_const_ehl,
-	.clock_freq = 200000000,
-};
-
 static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 {
 	struct device *dev = &pci->dev;
-	const struct m_can_pci_config *cfg;
 	struct m_can_classdev *mcan_class;
 	struct m_can_pci_priv *priv;
 	void __iomem *base;
@@ -150,8 +114,6 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	if (!mcan_class)
 		return -ENOMEM;
 
-	cfg = (const struct m_can_pci_config *)id->driver_data;
-
 	priv = cdev_to_priv(mcan_class);
 
 	priv->base = base;
@@ -163,9 +125,7 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	mcan_class->dev = &pci->dev;
 	mcan_class->net->irq = pci_irq_vector(pci, 0);
 	mcan_class->pm_clock_support = 1;
-	mcan_class->bit_timing = cfg->bit_timing;
-	mcan_class->data_timing = cfg->data_timing;
-	mcan_class->can.clock.freq = cfg->clock_freq;
+	mcan_class->can.clock.freq = id->driver_data;
 	mcan_class->ops = &m_can_pci_ops;
 
 	pci_set_drvdata(pci, mcan_class);
@@ -218,8 +178,8 @@ static SIMPLE_DEV_PM_OPS(m_can_pci_pm_ops,
 			 m_can_pci_suspend, m_can_pci_resume);
 
 static const struct pci_device_id m_can_pci_id_table[] = {
-	{ PCI_VDEVICE(INTEL, 0x4bc1), (kernel_ulong_t)&m_can_pci_ehl, },
-	{ PCI_VDEVICE(INTEL, 0x4bc2), (kernel_ulong_t)&m_can_pci_ehl, },
+	{ PCI_VDEVICE(INTEL, 0x4bc1), M_CAN_CLOCK_FREQ_EHL, },
+	{ PCI_VDEVICE(INTEL, 0x4bc2), M_CAN_CLOCK_FREQ_EHL, },
 	{  }	/* Terminating Entry */
 };
 MODULE_DEVICE_TABLE(pci, m_can_pci_id_table);

base-commit: f3f19f939c11925dadd3f4776f99f8c278a7017b
-- 
2.35.1

Re: [PATCH net 1/2] Revert "can: m_can: pci: use custom bit timings for Elkhart Lake"

[Jakub Kicinski]

On Fri, 13 May 2022 15:08:18 +0200 Marc Kleine-Budde wrote:
> From: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> This reverts commit 0e8ffdf3b86dfd44b651f91b12fcae76c25c453b.
> 
> Commit 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for
> Elkhart Lake") broke the test case using bitrate switching.
> 
> | ip link set can0 up type can bitrate 500000 dbitrate 4000000 fd on
> | ip link set can1 up type can bitrate 500000 dbitrate 4000000 fd on
> | candump can0 &
> | cangen can1 -I 0x800 -L 64 -e -fb \
> |     -D 11223344deadbeef55667788feedf00daabbccdd44332211 -n 1 -v -v
> 
> Above commit does everything correctly according to the datasheet.
> However datasheet wasn't correct.
> 
> I got confirmation from hardware engineers that the actual CAN
> hardware on Intel Elkhart Lake is based on M_CAN version v3.2.0.
> Datasheet was mirroring values from an another specification which was
> based on earlier M_CAN version leading to wrong bit timings.
> 
> Therefore revert the commit and switch back to common bit timings.
> 
> Fixes: 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for Elkhart Lake")
> Link: https://lore.kernel.org/all/20220512124144.536850-1-jarkko.nikula@linux.intel.com
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Reported-by: Chee Hou Ong <chee.houx.ong@intel.com>
> Reported-by: Aman Kumar <aman.kumar@intel.com>
> Reported-by: Pallavi Kumari <kumari.pallavi@intel.com>
> Cc: <stable@vger.kernel.org> # v5.16+
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

nit: the hash in the fixes tag should be:

Fixes: ea4c1787685d ("can: m_can: pci: use custom bit timings for Elkhart Lake")

Do you want to respin or is the can tree non-rebasable?

Re: [PATCH net 1/2] Revert "can: m_can: pci: use custom bit timings for Elkhart Lake"

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

On 13.05.2022 10:21:45, Jakub Kicinski wrote:
> On Fri, 13 May 2022 15:08:18 +0200 Marc Kleine-Budde wrote:
> > From: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > 
> > This reverts commit 0e8ffdf3b86dfd44b651f91b12fcae76c25c453b.
> > 
> > Commit 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for
> > Elkhart Lake") broke the test case using bitrate switching.
> > 
> > | ip link set can0 up type can bitrate 500000 dbitrate 4000000 fd on
> > | ip link set can1 up type can bitrate 500000 dbitrate 4000000 fd on
> > | candump can0 &
> > | cangen can1 -I 0x800 -L 64 -e -fb \
> > |     -D 11223344deadbeef55667788feedf00daabbccdd44332211 -n 1 -v -v
> > 
> > Above commit does everything correctly according to the datasheet.
> > However datasheet wasn't correct.
> > 
> > I got confirmation from hardware engineers that the actual CAN
> > hardware on Intel Elkhart Lake is based on M_CAN version v3.2.0.
> > Datasheet was mirroring values from an another specification which was
> > based on earlier M_CAN version leading to wrong bit timings.
> > 
> > Therefore revert the commit and switch back to common bit timings.
> > 
> > Fixes: 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for Elkhart Lake")
> > Link: https://lore.kernel.org/all/20220512124144.536850-1-jarkko.nikula@linux.intel.com
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Reported-by: Chee Hou Ong <chee.houx.ong@intel.com>
> > Reported-by: Aman Kumar <aman.kumar@intel.com>
> > Reported-by: Pallavi Kumari <kumari.pallavi@intel.com>
> > Cc: <stable@vger.kernel.org> # v5.16+
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> nit: the hash in the fixes tag should be:

Doh!

> Fixes: ea4c1787685d ("can: m_can: pci: use custom bit timings for Elkhart Lake")
> 
> Do you want to respin or is the can tree non-rebasable?

No - it's non-rebasable as soon as merged to net(-next) :)

Here's a new pull request with a adjusted Fixes tag.

| https://lore.kernel.org/all/20220514185742.407230-1-mkl@pengutronix.de

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 1/2] Revert "can: m_can: pci: use custom bit timings for Elkhart Lake"

[Jarkko Nikula]

Hi

Sorry the late response. I was offline a few days.

On 5/13/22 20:21, Jakub Kicinski wrote:
>> Fixes: 0e8ffdf3b86d ("can: m_can: pci: use custom bit timings for Elkhart Lake")
>> Link: https://lore.kernel.org/all/20220512124144.536850-1-jarkko.nikula@linux.intel.com
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Reported-by: Chee Hou Ong <chee.houx.ong@intel.com>
>> Reported-by: Aman Kumar <aman.kumar@intel.com>
>> Reported-by: Pallavi Kumari <kumari.pallavi@intel.com>
>> Cc: <stable@vger.kernel.org> # v5.16+
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> nit: the hash in the fixes tag should be:
> 
> Fixes: ea4c1787685d ("can: m_can: pci: use custom bit timings for Elkhart Lake")
> 
> Do you want to respin or is the can tree non-rebasable?

Grr... Looks like I took accidentally linux-stable commit Id. Obviously 
too hurry to vacation :-|

Thanks for fixing it up Marc!

PCH_CAN removal?

[Oliver Hartkopp]

Hi Jarkko,

I'm very happy to see that you are cleaning up some custom M_CAN stuff 
which turns out to be already supported ;-)

In fact there is a pch_can (PCI) driver which was used in an Intel 
embedded setup "Topcliff PCH" ~11 years ago. The driver was originally 
contributed by OKI.

And I was pretty sure that this driver implements a C_CAN IP core with a 
PCI binding which is now implemented in the C_CAN_PCI driver.

As I do not have that hardware anymore it would be interesting to see if 
you could bring the PCH board to life with the C_CAN_PCI driver?!?

That would allow us to remove the (then definitely) obsolete pch_can driver.

Thanks & best regards,
Oliver

Re: PCH_CAN removal?

[Jarkko Nikula]

Hi

On 5/19/22 22:15, Oliver Hartkopp wrote:
> In fact there is a pch_can (PCI) driver which was used in an Intel 
> embedded setup "Topcliff PCH" ~11 years ago. The driver was originally 
> contributed by OKI.
> 
> And I was pretty sure that this driver implements a C_CAN IP core with a 
> PCI binding which is now implemented in the C_CAN_PCI driver.
> 
> As I do not have that hardware anymore it would be interesting to see if 
> you could bring the PCH board to life with the C_CAN_PCI driver?!?
> 
> That would allow us to remove the (then definitely) obsolete pch_can 
> driver.
> 
I'm afraid we don't have that HW either here locally :-(

Jarkko

Re: PCH_CAN removal?

[Oliver Hartkopp]

Hi Jarkko,

thanks for your fast reply!

On 20.05.22 14:48, Jarkko Nikula wrote:

> On 5/19/22 22:15, Oliver Hartkopp wrote:

>> As I do not have that hardware anymore it would be interesting to see 
>> if you could bring the PCH board to life with the C_CAN_PCI driver?!?
>>
>> That would allow us to remove the (then definitely) obsolete pch_can 
>> driver.
>>
> I'm afraid we don't have that HW either here locally :-(

Hm, sad. At least it was a try to get the pch_can removed ;-)

Many thanks & best regards,
Oliver

ps. to whom it may concern: the two mail addresses from the OKISEMI 
employees bounced at sending :-[

[PATCH net 2/2] can: m_can: remove support for custom bit timing, take #2

[Marc Kleine-Budde]

From: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Now when Intel Elkhart Lake uses again common bit timing and there are
no other users for custom bit timing, we can bring back the changes
done by the commit 0ddd83fbebbc ("can: m_can: remove support for
custom bit timing").

This effectively reverts commit ea768b2ffec6 ("Revert "can: m_can:
remove support for custom bit timing"") while taking into account
commit ea22ba40debe ("can: m_can: make custom bittiming fields const")
and commit 7d4a101c0bd3 ("can: dev: add sanity check in
can_set_static_ctrlmode()").

Link: https://lore.kernel.org/all/20220512124144.536850-2-jarkko.nikula@linux.intel.com
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 24 ++++++------------------
 drivers/net/can/m_can/m_can.h |  3 ---
 2 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index b3b5bc1c803b..088bb1bcf1ef 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1495,34 +1495,22 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
 		err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
 		if (err)
 			return err;
-		cdev->can.bittiming_const = cdev->bit_timing ?
-			cdev->bit_timing : &m_can_bittiming_const_30X;
-
-		cdev->can.data_bittiming_const = cdev->data_timing ?
-			cdev->data_timing :
-			&m_can_data_bittiming_const_30X;
+		cdev->can.bittiming_const = &m_can_bittiming_const_30X;
+		cdev->can.data_bittiming_const = &m_can_data_bittiming_const_30X;
 		break;
 	case 31:
 		/* CAN_CTRLMODE_FD_NON_ISO is fixed with M_CAN IP v3.1.x */
 		err = can_set_static_ctrlmode(dev, CAN_CTRLMODE_FD_NON_ISO);
 		if (err)
 			return err;
-		cdev->can.bittiming_const = cdev->bit_timing ?
-			cdev->bit_timing : &m_can_bittiming_const_31X;
-
-		cdev->can.data_bittiming_const = cdev->data_timing ?
-			cdev->data_timing :
-			&m_can_data_bittiming_const_31X;
+		cdev->can.bittiming_const = &m_can_bittiming_const_31X;
+		cdev->can.data_bittiming_const = &m_can_data_bittiming_const_31X;
 		break;
 	case 32:
 	case 33:
 		/* Support both MCAN version v3.2.x and v3.3.0 */
-		cdev->can.bittiming_const = cdev->bit_timing ?
-			cdev->bit_timing : &m_can_bittiming_const_31X;
-
-		cdev->can.data_bittiming_const = cdev->data_timing ?
-			cdev->data_timing :
-			&m_can_data_bittiming_const_31X;
+		cdev->can.bittiming_const = &m_can_bittiming_const_31X;
+		cdev->can.data_bittiming_const = &m_can_data_bittiming_const_31X;
 
 		cdev->can.ctrlmode_supported |=
 			(m_can_niso_supported(cdev) ?
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 2c5d40997168..d18b515e6ccc 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -85,9 +85,6 @@ struct m_can_classdev {
 	struct sk_buff *tx_skb;
 	struct phy *transceiver;
 
-	const struct can_bittiming_const *bit_timing;
-	const struct can_bittiming_const *data_timing;
-
 	struct m_can_ops *ops;
 
 	int version;
-- 
2.35.1