Re: Reg: New MFD Driver for my PCIe Device

Re: Reg: New MFD Driver for my PCIe Device

[Lee Jones]

On Mon, 08 Nov 2021, Kumaravel.Thiagarajan@microchip.com wrote:

> Dear Lee Jones,
> 
> I am Kumaravel Thiagarajan from Microchip, India and I am new to Linux Kernel development.
> 
> I am currently working on linux kernel driver for one of our PCIe based devices whose BAR 0 maps interface registers for a gpio controller, an OTP memory device controller and an EEPROM device controller into the host processor's memory space.
> 
> Based on earlier inputs from Linus Walleij, I have developed this as a multi-function device driver - First MFD driver (drivers/mfd) gets loaded for the PCIe device and then it spawns two child devices for OTP/EEPROM and GPIO separately.

You may wish to speak with Greg about your architectural decisions.

He usually dislikes the creation of platform devices from PCI ones.

> I have four new files in my local linux source tree as below for community submission.
> 
> 1. mchp_pci1xxxx_gp.h under include/linux/mfd
> 2. mchp_pci1xxxx_gp.c under drivers/mfd
> 3. mchp_pci1xxxx_gpio.c under drivers/gpio
> 4. mchp_pci1xxxx_ otpe2p.c under drivers/mtd/devices
> 
> I have a doubt with the location of the fourth file for the following reasons.
> 
> 1. I have visualized and architected the OTP and EEPROM memories of my device as individual block devices so that I am able to use the linux dd command to program these memories from a binary file directly or edit them directly using applications like hexedit, both of which I am able to do.
> 2. These only look more like block devices to me and not MTD devices (only writes and reads are possible and not erase).
> 
> Can I move the fourth file directly under drivers/block or should there be a new sub directory under drivers/block or leave it as such under drivers/mtd/devices ?

Those are questions for the MTD/Block Maintainers.

I've Cc'ed all of the relevant parties for you.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Reg: New MFD Driver for my PCIe Device

[Greg KH]

On Mon, Nov 08, 2021 at 11:04:31AM +0000, Lee Jones wrote:
> On Mon, 08 Nov 2021, Kumaravel.Thiagarajan@microchip.com wrote:
> 
> > Dear Lee Jones,
> > 
> > I am Kumaravel Thiagarajan from Microchip, India and I am new to Linux Kernel development.
> > 
> > I am currently working on linux kernel driver for one of our PCIe based devices whose BAR 0 maps interface registers for a gpio controller, an OTP memory device controller and an EEPROM device controller into the host processor's memory space.
> > 
> > Based on earlier inputs from Linus Walleij, I have developed this as a multi-function device driver - First MFD driver (drivers/mfd) gets loaded for the PCIe device and then it spawns two child devices for OTP/EEPROM and GPIO separately.
> 
> You may wish to speak with Greg about your architectural decisions.
> 
> He usually dislikes the creation of platform devices from PCI ones.

Yes, that is NOT ok.

Platform devices are only for devices that are actually on a platform
(i.e. described by DT or other firmware types).  PCI devices are NOT
platform devices, please use the correct apis for this instead (i.e. the
aux bus)

thanks,

greg k-h

Re: Reg: New MFD Driver for my PCIe Device

[Lee Jones]

On Mon, 08 Nov 2021, Greg KH wrote:
> On Mon, Nov 08, 2021 at 11:04:31AM +0000, Lee Jones wrote:
> > On Mon, 08 Nov 2021, Kumaravel.Thiagarajan@microchip.com wrote:
> > 
> > > Dear Lee Jones,
> > > 
> > > I am Kumaravel Thiagarajan from Microchip, India and I am new to Linux Kernel development.
> > > 
> > > I am currently working on linux kernel driver for one of our PCIe based devices whose BAR 0 maps interface registers for a gpio controller, an OTP memory device controller and an EEPROM device controller into the host processor's memory space.
> > > 
> > > Based on earlier inputs from Linus Walleij, I have developed this as a multi-function device driver - First MFD driver (drivers/mfd) gets loaded for the PCIe device and then it spawns two child devices for OTP/EEPROM and GPIO separately.
> > 
> > You may wish to speak with Greg about your architectural decisions.
> > 
> > He usually dislikes the creation of platform devices from PCI ones.
> 
> Yes, that is NOT ok.
> 
> Platform devices are only for devices that are actually on a platform
> (i.e. described by DT or other firmware types).

This is probably a bit of an over-simplification.  Lots of legitimate
platform devices are actually described by DT et al.

However, it is true that devices which reside on definite buses; PCI,
USB, PCMIA, SCSI, Thunderbolt, etc should not spawn their children off
as platform devices.

> PCI devices are NOT
> platform devices, please use the correct apis for this instead (i.e. the
> aux bus)

Grep for "auxiliary_device".

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: Reg: New MFD Driver for my PCIe Device

[Greg KH]

On Mon, Nov 08, 2021 at 11:27:45AM +0000, Lee Jones wrote:
> On Mon, 08 Nov 2021, Greg KH wrote:
> > On Mon, Nov 08, 2021 at 11:04:31AM +0000, Lee Jones wrote:
> > > On Mon, 08 Nov 2021, Kumaravel.Thiagarajan@microchip.com wrote:
> > > 
> > > > Dear Lee Jones,
> > > > 
> > > > I am Kumaravel Thiagarajan from Microchip, India and I am new to Linux Kernel development.
> > > > 
> > > > I am currently working on linux kernel driver for one of our PCIe based devices whose BAR 0 maps interface registers for a gpio controller, an OTP memory device controller and an EEPROM device controller into the host processor's memory space.
> > > > 
> > > > Based on earlier inputs from Linus Walleij, I have developed this as a multi-function device driver - First MFD driver (drivers/mfd) gets loaded for the PCIe device and then it spawns two child devices for OTP/EEPROM and GPIO separately.
> > > 
> > > You may wish to speak with Greg about your architectural decisions.
> > > 
> > > He usually dislikes the creation of platform devices from PCI ones.
> > 
> > Yes, that is NOT ok.
> > 
> > Platform devices are only for devices that are actually on a platform
> > (i.e. described by DT or other firmware types).
> 
> This is probably a bit of an over-simplification.  Lots of legitimate
> platform devices are actually described by DT et al.

We are in violent agreement here, that is what I was trying to say :)

> However, it is true that devices which reside on definite buses; PCI,
> USB, PCMIA, SCSI, Thunderbolt, etc should not spawn their children off
> as platform devices.

Agreed, that is not ok, as those are not what the platform device code
was designed for.

thanks,

greg k-h

RE: Reg: New MFD Driver for my PCIe Device

[Kumaravel.Thiagarajan]

Dear Greg K-H & Lee Jones,

Thanks for your inputs and I need more of your help to understand things better.

I took this MFD route not just based on the recommendation from Linus Walleij but also based on the kernel documentation @ /Documentation/driver-api/driver-model/platform.rst which states as below.

"Rarely, a platform_device will be connected through a segment of some other kind of bus; but its registers will still be directly addressable."

I visualized these two (GPIO controller & OTP/EEPROM controller) devices as platform devices present on the same PCI function and these two devices are not detectable unless the base PCI function driver enumerates them and anyway their registers are directly addressable.
Hence, I thought that the platform driver architecture is inclusive of devices like this.

Please let me know your comments.

Also please let me know if I can talk to any of you over a webex call to get clarifications on my further doubts.

Thank You.

Regards,
Kumaravel Thiagarajan

-----Original Message-----
From: Greg KH <gregkh@linuxfoundation.org> 
Sent: Monday, November 8, 2021 5:33 PM
To: Lee Jones <lee.jones@linaro.org>
Cc: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>; Pragash Mangalapandian - I21326 <Pragash.Mangalapandian@microchip.com>; Sundararaman Hariharaputran - I21286 <Sundararaman.H@microchip.com>; axboe@kernel.dk; linux-block@vger.kernel.org; miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com; linux-mtd@lists.infradead.org
Subject: Re: Reg: New MFD Driver for my PCIe Device

[You don't often get email from gregkh@linuxfoundation.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Mon, Nov 08, 2021 at 11:27:45AM +0000, Lee Jones wrote:
> On Mon, 08 Nov 2021, Greg KH wrote:
> > On Mon, Nov 08, 2021 at 11:04:31AM +0000, Lee Jones wrote:
> > > On Mon, 08 Nov 2021, Kumaravel.Thiagarajan@microchip.com wrote:
> > >
> > > > Dear Lee Jones,
> > > >
> > > > I am Kumaravel Thiagarajan from Microchip, India and I am new to Linux Kernel development.
> > > >
> > > > I am currently working on linux kernel driver for one of our PCIe based devices whose BAR 0 maps interface registers for a gpio controller, an OTP memory device controller and an EEPROM device controller into the host processor's memory space.
> > > >
> > > > Based on earlier inputs from Linus Walleij, I have developed this as a multi-function device driver - First MFD driver (drivers/mfd) gets loaded for the PCIe device and then it spawns two child devices for OTP/EEPROM and GPIO separately.
> > >
> > > You may wish to speak with Greg about your architectural decisions.
> > >
> > > He usually dislikes the creation of platform devices from PCI ones.
> >
> > Yes, that is NOT ok.
> >
> > Platform devices are only for devices that are actually on a 
> > platform (i.e. described by DT or other firmware types).
>
> This is probably a bit of an over-simplification.  Lots of legitimate 
> platform devices are actually described by DT et al.

We are in violent agreement here, that is what I was trying to say :)

> However, it is true that devices which reside on definite buses; PCI, 
> USB, PCMIA, SCSI, Thunderbolt, etc should not spawn their children off 
> as platform devices.

Agreed, that is not ok, as those are not what the platform device code was designed for.

thanks,

greg k-h

Re: Reg: New MFD Driver for my PCIe Device

[Greg KH]

On Tue, Nov 16, 2021 at 11:34:24AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> Dear Greg K-H & Lee Jones,
> 
> Thanks for your inputs and I need more of your help to understand things better.
> 
> I took this MFD route not just based on the recommendation from Linus Walleij but also based on the kernel documentation @ /Documentation/driver-api/driver-model/platform.rst which states as below.
> 
> "Rarely, a platform_device will be connected through a segment of some other kind of bus; but its registers will still be directly addressable."
> 
> I visualized these two (GPIO controller & OTP/EEPROM controller) devices as platform devices present on the same PCI function and these two devices are not detectable unless the base PCI function driver enumerates them and anyway their registers are directly addressable.
> Hence, I thought that the platform driver architecture is inclusive of devices like this.

Sorry, but no.  Again, platform devices are ONLY for actual platform
devices, not "things on a device that happens to be on another bus
device".  Like PCI devices.

That is what the auxiliary bus code was written for, please read
Documentation/driver-api/auxiliary_bus.rst for how to use it.

> Please let me know your comments.
> 
> Also please let me know if I can talk to any of you over a webex call to get clarifications on my further doubts.

Email works best, video chats for every patch review does not scale at
all :)

thanks,

greg k-h

RE: Reg: New MFD Driver for my PCIe Device

[Kumaravel.Thiagarajan]

Dear Greg KH,

I went through the documentation of aux bus and felt that it would be the correct way to go as you said.
I will migrate from MFD to aux bus. 

I have one more architectural question as below.
I have written the driver such that it enumerates the OTP memory and EEPROM memory as two separate block devices or disks each of size 8KB and this enables me to use the linux dd command with "direct" option to dump the configuration binary onto OTP or EEPROM devices.
Also, this enables me to use the application like hexedit to view the OTP or EEPROM devices in raw binary format.
These devices are not based on mtd (memory technology device) architecture as we don't have any erase functionality here.
Can you please let me know a suitable location in kernel source tree for my block or disk device driver?

Thank You.

Regards,
Kumaravel Thiagarajan

-----Original Message-----
From: Greg KH <gregkh@linuxfoundation.org> 
Sent: Tuesday, November 16, 2021 7:50 PM
To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
Cc: lee.jones@linaro.org; Pragash Mangalapandian - I21326 <Pragash.Mangalapandian@microchip.com>; Sundararaman Hariharaputran - I21286 <Sundararaman.H@microchip.com>; axboe@kernel.dk; linux-block@vger.kernel.org; miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com; linux-mtd@lists.infradead.org; Lakshmi Praveen Kopparthi - I17972 <LakshmiPraveen.Kopparthi@microchip.com>
Subject: Re: Reg: New MFD Driver for my PCIe Device

[You don't often get email from gregkh@linuxfoundation.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.]

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Tue, Nov 16, 2021 at 11:34:24AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> Dear Greg K-H & Lee Jones,
>
> Thanks for your inputs and I need more of your help to understand things better.
>
> I took this MFD route not just based on the recommendation from Linus Walleij but also based on the kernel documentation @ /Documentation/driver-api/driver-model/platform.rst which states as below.
>
> "Rarely, a platform_device will be connected through a segment of some other kind of bus; but its registers will still be directly addressable."
>
> I visualized these two (GPIO controller & OTP/EEPROM controller) devices as platform devices present on the same PCI function and these two devices are not detectable unless the base PCI function driver enumerates them and anyway their registers are directly addressable.
> Hence, I thought that the platform driver architecture is inclusive of devices like this.

Sorry, but no.  Again, platform devices are ONLY for actual platform devices, not "things on a device that happens to be on another bus device".  Like PCI devices.

That is what the auxiliary bus code was written for, please read Documentation/driver-api/auxiliary_bus.rst for how to use it.

> Please let me know your comments.
>
> Also please let me know if I can talk to any of you over a webex call to get clarifications on my further doubts.

Email works best, video chats for every patch review does not scale at all :)

thanks,

greg k-h

Re: Reg: New MFD Driver for my PCIe Device

[Greg KH]

On Fri, Nov 19, 2021 at 09:16:29AM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> Dear Greg KH,
> 
> I went through the documentation of aux bus and felt that it would be the correct way to go as you said.
> I will migrate from MFD to aux bus. 
> 
> I have one more architectural question as below.
> I have written the driver such that it enumerates the OTP memory and EEPROM memory as two separate block devices or disks each of size 8KB and this enables me to use the linux dd command with "direct" option to dump the configuration binary onto OTP or EEPROM devices.
> Also, this enables me to use the application like hexedit to view the OTP or EEPROM devices in raw binary format.
> These devices are not based on mtd (memory technology device) architecture as we don't have any erase functionality here.
> Can you please let me know a suitable location in kernel source tree for my block or disk device driver?

So they are a read-only block device?

Why use a block device and not just the "normal" eeprom driver?  Or a
char device node and mmap the memory?

Anyway, no idea where to put them, drivers/misc/ ?

thanks,

greg k-h

RE: Reg: New MFD Driver for my PCIe Device

[Kumaravel.Thiagarajan]

Dear Greg KH,

Please find my answers inline below and let me know if you have any questions or comments.

Thank You.

Regards,
Kumaravel Thiagarajan

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, November 19, 2021 8:23 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: lee.jones@linaro.org; Pragash Mangalapandian - I21326
> <Pragash.Mangalapandian@microchip.com>; Sundararaman Hariharaputran -
> I21286 <Sundararaman.H@microchip.com>; axboe@kernel.dk; linux-
> block@vger.kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> vigneshr@ti.com; linux-mtd@lists.infradead.org; Lakshmi Praveen Kopparthi
> - I17972 <LakshmiPraveen.Kopparthi@microchip.com>; Ronnie Kunin -
> C21729 <Ronnie.Kunin@microchip.com>
> Subject: Re: Reg: New MFD Driver for my PCIe Device
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Fri, Nov 19, 2021 at 09:16:29AM +0000,
> Kumaravel.Thiagarajan@microchip.com wrote:
> > Dear Greg KH,
> >
> > I went through the documentation of aux bus and felt that it would be the
> correct way to go as you said.
> > I will migrate from MFD to aux bus.
> >
> > I have one more architectural question as below.
> > I have written the driver such that it enumerates the OTP memory and
> EEPROM memory as two separate block devices or disks each of size 8KB and
> this enables me to use the linux dd command with "direct" option to dump
> the configuration binary onto OTP or EEPROM devices.
> > Also, this enables me to use the application like hexedit to view the OTP or
> EEPROM devices in raw binary format.
> > These devices are not based on mtd (memory technology device)
> architecture as we don't have any erase functionality here.
> > Can you please let me know a suitable location in kernel source tree for my
> block or disk device driver?
> 

> So they are a read-only block device?
KT: No. These block devices are writeable too. I program these block devices using the linux dd command with the "direct" option.

> 
> Why use a block device and not just the "normal" eeprom driver?
KT: I thought the normal EEPROM driver can be used only for EEPROMs that are connected to I2C adapters for which there is a I2C Adapter driver available already. But my device has an inbuilt EEPROM controller hardware which is not like an I2C adapter.

> Or a char device node and mmap the memory?
KT: I am not sure if I clearly understand this. Are you suggesting to mmap the EEPROM memory? The EEPROM memory is not mapped into the address space directly. EEPROM and OTP controller registers are mapped into the address space of the processor and accessed by the kernel mode driver.

> 
> Anyway, no idea where to put them, drivers/misc/ ?
KT: Ok. I can use this location.

> 
> thanks,
> 
> greg k-h

RE: Reg: New MFD Driver for my PCIe Device

[Kumaravel.Thiagarajan]

Dear Greg KH,

In case you missed my last email below, I have tried to answer your questions in that email.
Please let me know if you have any further questions or concerns.
This will be help me to finalize the architecture for my driver.

Thank You.

Regards,
Kumar

> -----Original Message-----
> From: Kumaravel Thiagarajan - I21417
> Sent: Wednesday, November 24, 2021 12:03 AM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: lee.jones@linaro.org; Pragash Mangalapandian - I21326
> <Pragash.Mangalapandian@microchip.com>; Sundararaman Hariharaputran -
> I21286 <Sundararaman.H@microchip.com>; axboe@kernel.dk; linux-
> block@vger.kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> vigneshr@ti.com; linux-mtd@lists.infradead.org; Lakshmi Praveen Kopparthi
> - I17972 <LakshmiPraveen.Kopparthi@microchip.com>; Ronnie Kunin -
> C21729 <Ronnie.Kunin@microchip.com>
> Subject: RE: Reg: New MFD Driver for my PCIe Device
> 
> Dear Greg KH,
> 
> Please find my answers inline below and let me know if you have any
> questions or comments.
> 
> Thank You.
> 
> Regards,
> Kumaravel Thiagarajan
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Friday, November 19, 2021 8:23 PM
> > To: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@microchip.com>
> > Cc: lee.jones@linaro.org; Pragash Mangalapandian - I21326
> > <Pragash.Mangalapandian@microchip.com>; Sundararaman
> Hariharaputran -
> > I21286 <Sundararaman.H@microchip.com>; axboe@kernel.dk; linux-
> > block@vger.kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> > vigneshr@ti.com; linux-mtd@lists.infradead.org; Lakshmi Praveen
> > Kopparthi
> > - I17972 <LakshmiPraveen.Kopparthi@microchip.com>; Ronnie Kunin -
> > C21729 <Ronnie.Kunin@microchip.com>
> > Subject: Re: Reg: New MFD Driver for my PCIe Device
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > the content is safe
> >
> > On Fri, Nov 19, 2021 at 09:16:29AM +0000,
> > Kumaravel.Thiagarajan@microchip.com wrote:
> > > Dear Greg KH,
> > >
> > > I went through the documentation of aux bus and felt that it would
> > > be the
> > correct way to go as you said.
> > > I will migrate from MFD to aux bus.
> > >
> > > I have one more architectural question as below.
> > > I have written the driver such that it enumerates the OTP memory and
> > EEPROM memory as two separate block devices or disks each of size 8KB
> > and this enables me to use the linux dd command with "direct" option
> > to dump the configuration binary onto OTP or EEPROM devices.
> > > Also, this enables me to use the application like hexedit to view
> > > the OTP or
> > EEPROM devices in raw binary format.
> > > These devices are not based on mtd (memory technology device)
> > architecture as we don't have any erase functionality here.
> > > Can you please let me know a suitable location in kernel source tree
> > > for my
> > block or disk device driver?
> >
> 
> > So they are a read-only block device?
> KT: No. These block devices are writeable too. I program these block devices
> using the linux dd command with the "direct" option.
> 
> >
> > Why use a block device and not just the "normal" eeprom driver?
> KT: I thought the normal EEPROM driver can be used only for EEPROMs that
> are connected to I2C adapters for which there is a I2C Adapter driver
> available already. But my device has an inbuilt EEPROM controller hardware
> which is not like an I2C adapter.
> 
> > Or a char device node and mmap the memory?
> KT: I am not sure if I clearly understand this. Are you suggesting to mmap the
> EEPROM memory? The EEPROM memory is not mapped into the address
> space directly. EEPROM and OTP controller registers are mapped into the
> address space of the processor and accessed by the kernel mode driver.
> 
> >
> > Anyway, no idea where to put them, drivers/misc/ ?
> KT: Ok. I can use this location.
> 
> >
> > thanks,
> >
> > greg k-h

Re: Reg: New MFD Driver for my PCIe Device

[Greg KH]

On Tue, Nov 30, 2021 at 05:42:14PM +0000, Kumaravel.Thiagarajan@microchip.com wrote:
> Dear Greg KH,
> 
> In case you missed my last email below, I have tried to answer your questions in that email.
> Please let me know if you have any further questions or concerns.

At the moment, no, I was waiting for patches to comment further.

thanks,

greg k-h