RE: [PATCH v1] platform/mellanox: Add Mellanox TRIO driver

[Vadim Pasternak]

> -----Original Message-----
> From: Shravan Ramani
> Sent: Wednesday, November 13, 2019 7:41 AM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; Andy Shevchenko
> <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>
> Cc: Liming Sun <lsun@xxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v1] platform/mellanox: Add Mellanox TRIO driver
> 
> Hi Vadim,
> TRIO stands for TRansaction I/O and is an internal code name for our PCIe to
> CHI bus interface.
> In Mellanox BlueField SoC, the configuration is as follows: there are 3 TRIO
> blocks where TRIO2 is connected to a Mellanox ConnectX-5  while TRIO0 and
> TRIO1 can be configured to behave either as a PCIe Root Complex to
> downstream ports (8 ports or 16 lanes each) connecting to storage devices, or
> as an end-point when plugged into an external x86 host (SmartNIC form factor).
> Each TRIO block has a separate ACPI table entry which invokes this driver
> thereby creating a total of 3 instances.
> The purpose of this driver is to be able to read/set the L3 cache profile from a
> list of available profiles for transactions coming in to each TRIO block and is
> meant to run on the ARM cores powering the BlueField SoC.

OK.
It was necessary to have such sort of explanation in the description.

I am also not sure you choose correct location for this driver.
Why it should be in drivers/platform/mellanox/ and not in for example
drivers/acpi/?

Regarding the patch content.
(1)
Please, follow naming convention which we have in folder:
drivers/platform/mellanox/, like in module mlxbf-tmfifo.c
mlxbf_trio
MLXBF_TRIO
for all routine names, defines, types.
(2)
Fix includes order ("io" before "irq").
(3)
Don't use magic numbers like '3'.
(4)
Why in 'probe' routine failure of symlink creation treated as
warning? 
(5)
Remove stuff like
'dev_warn(dev, "%s: failed to find reg resource 0\n", __func__);'
'dev_warn(dev, "Error probing trio\n");'
(6)
I suggest to make all the above comments and send the updated
patch for internal review to myself cced to linux-internal@xxxxxxxxxxxx,
before sending it to linux-internal@xxxxxxxxxxxxx

Thanks,
Vadim.

> 
> Regards,
> Shravan
> 
> -----Original Message-----
> From: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Sent: Tuesday, November 12, 2019 8:15 PM
> To: Shravan Ramani <sramani@xxxxxxxxxxxx>; Andy Shevchenko
> <andy@xxxxxxxxxxxxx>; Darren Hart <dvhart@xxxxxxxxxxxxx>
> Cc: Liming Sun <lsun@xxxxxxxxxxxx>; Shravan Ramani
> <sramani@xxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v1] platform/mellanox: Add Mellanox TRIO driver
> 
> 
> 
> > -----Original Message-----
> > From: Shravan Kumar Ramani <sramani@xxxxxxxxxxxx>
> > Sent: Monday, November 11, 2019 4:35 PM
> > To: Andy Shevchenko <andy@xxxxxxxxxxxxx>; Darren Hart
> > <dvhart@xxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> > Cc: Liming Sun <lsun@xxxxxxxxxxxx>; Shravan Ramani
> > <sramani@xxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx
> > Subject: [PATCH v1] platform/mellanox: Add Mellanox TRIO driver
> >
> > This patch adds support for Mellanox BlueField TRIO PCIe host controller.
> > The driver supports multiple TRIO instances and provides a sysfs
> > interface to allow the user to read/set the L3 cache profile for
> > transactions going through the TRIO. It also provides an interrupt handler for
> the TRIO blocks.
> 
> Hi Shravan,
> 
> Could you, please, explain what TRIO PCIe host controller?
> What is TRIO, is it some internal name or it's some standard terminology?
> If it's internal, please, explain for what it stands for.
> 
> Same for TRIO instances. Are there some host side PCI instances?
> What are the purpose of them?
> 
> Could you, please, also explain the system configuration?
> 
> 
> >
> > Shravan Kumar Ramani (1):
> >   platform/mellanox: Add Mellanox TRIO driver
> >
> >  MAINTAINERS                            |   5 +
> >  drivers/platform/mellanox/Kconfig      |   8 +
> >  drivers/platform/mellanox/Makefile     |   1 +
> >  drivers/platform/mellanox/mlxbf-trio.c | 624
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 638 insertions(+)
> >  create mode 100644 drivers/platform/mellanox/mlxbf-trio.c
> >
> > --
> > 2.1.2