| Message ID | ade2c1a8d1eb48681b01ccdc9bed22caf5e96c4b.1542904564.git.soares@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | i3c: master: dw: split dw-i3c-master.c into master and bus specific parts | expand |
On Thu, 22 Nov 2018 17:54:54 +0000 Vitor Soares <vitor.soares@synopsys.com> wrote: > From: Vitor Soares <soares@synopsys.com> > > This patch slipts dw-i3c-master.c into three pieces: > dw-i3c-master.c - contains the code that interacts directly with the > core in master mode. > > dw-i3c-platdrv.c - contains the code specific to the platform driver. > > dw-i3c-core.h - contains the definitions and declarations shared by > dw-i3c-master and dw-i3c-platdrv > > This patch will allow SOC integrators to add their code specific to > DesignWare I3C IP. Isn't it too early to do this change? Can't we wait until we have a SoC that actually embeds this IP? > > Signed-off-by: Vitor Soares <soares@synopsys.com> > --- > drivers/i3c/master/Kconfig | 9 +- > drivers/i3c/master/Makefile | 5 +- > drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ > drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- > drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ I'd prefer to have a dw/ subdir where you'd place all dw files. > 5 files changed, 349 insertions(+), 290 deletions(-) > create mode 100644 drivers/i3c/master/dw-i3c-core.h > create mode 100644 drivers/i3c/master/dw-i3c-platdrv.c > > diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig > index 8ee1ce6..fdc6e46 100644 > --- a/drivers/i3c/master/Kconfig > +++ b/drivers/i3c/master/Kconfig > @@ -5,9 +5,14 @@ config CDNS_I3C_MASTER > help > Enable this driver if you want to support Cadence I3C master block. > > -config DW_I3C_MASTER > - tristate "Synospsys DesignWare I3C master driver" > +config DW_I3C_CORE > + tristate > + > +config DW_I3C_PLATFORM > + tristate "Synospsys DesignWare I3C Platform driver" > + select DW_I3C_CORE > depends on I3C > + depends on HAS_IOMEM > depends on !(ALPHA || PARISC) > # ALPHA and PARISC needs {read,write}sl() > help > diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile > index fc53939..004ad1c 100644 > --- a/drivers/i3c/master/Makefile > +++ b/drivers/i3c/master/Makefile > @@ -1,2 +1,5 @@ > obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o > -obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o > +obj-$(CONFIG_DW_I3C_CORE) += dw-i3c-core.o > +dw-i3c-core-objs := dw-i3c-master.o > +obj-$(CONFIG_DW_I3C_PLATFORM) += dw-i3c-platform.o > +dw-i3c-platform-objs := dw-i3c-platdrv.o Do we really have to create one module for the core and one per SoC? Can't we have everything in the same .ko?
Hi Boris, On 22/11/18 20:02, Boris Brezillon wrote: > On Thu, 22 Nov 2018 17:54:54 +0000 > Vitor Soares <vitor.soares@synopsys.com> wrote: > >> From: Vitor Soares <soares@synopsys.com> >> >> This patch slipts dw-i3c-master.c into three pieces: >> dw-i3c-master.c - contains the code that interacts directly with the >> core in master mode. >> >> dw-i3c-platdrv.c - contains the code specific to the platform driver. >> >> dw-i3c-core.h - contains the definitions and declarations shared by >> dw-i3c-master and dw-i3c-platdrv >> >> This patch will allow SOC integrators to add their code specific to >> DesignWare I3C IP. > Isn't it too early to do this change? Can't we wait until we have a SoC > that actually embeds this IP? I'm trying to turn it more flexible so the other can reuse the code. > >> Signed-off-by: Vitor Soares <soares@synopsys.com> >> --- >> drivers/i3c/master/Kconfig | 9 +- >> drivers/i3c/master/Makefile | 5 +- >> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ >> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- >> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ > I'd prefer to have a dw/ subdir where you'd place all dw files. Sure. I will change to this: ../dwc |-core.h |-master.c |-platdrv.c so the user doesn't need to write dw-i3c.. several times. The folder name is the same as for other subsystem (e.g. PCI). What do you think? > >> 5 files changed, 349 insertions(+), 290 deletions(-) >> create mode 100644 drivers/i3c/master/dw-i3c-core.h >> create mode 100644 drivers/i3c/master/dw-i3c-platdrv.c >> >> diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig >> index 8ee1ce6..fdc6e46 100644 >> --- a/drivers/i3c/master/Kconfig >> +++ b/drivers/i3c/master/Kconfig >> @@ -5,9 +5,14 @@ config CDNS_I3C_MASTER >> help >> Enable this driver if you want to support Cadence I3C master block. >> >> -config DW_I3C_MASTER >> - tristate "Synospsys DesignWare I3C master driver" >> +config DW_I3C_CORE >> + tristate >> + >> +config DW_I3C_PLATFORM >> + tristate "Synospsys DesignWare I3C Platform driver" >> + select DW_I3C_CORE >> depends on I3C >> + depends on HAS_IOMEM >> depends on !(ALPHA || PARISC) >> # ALPHA and PARISC needs {read,write}sl() >> help >> diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile >> index fc53939..004ad1c 100644 >> --- a/drivers/i3c/master/Makefile >> +++ b/drivers/i3c/master/Makefile >> @@ -1,2 +1,5 @@ >> obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o >> -obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o >> +obj-$(CONFIG_DW_I3C_CORE) += dw-i3c-core.o >> +dw-i3c-core-objs := dw-i3c-master.o >> +obj-$(CONFIG_DW_I3C_PLATFORM) += dw-i3c-platform.o >> +dw-i3c-platform-objs := dw-i3c-platdrv.o > Do we really have to create one module for the core and one per SoC? > Can't we have everything in the same .ko? This will help the introduction of new modules. The design in my mind is to have: -core.h -common.c -master.c -slave.c ... I'm not sure if make sense to change core.h to common.h. Thaks for your feedback. Best regards, Vitor Soares
On Fri, 23 Nov 2018 12:39:31 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris, > > > On 22/11/18 20:02, Boris Brezillon wrote: > > On Thu, 22 Nov 2018 17:54:54 +0000 > > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > >> From: Vitor Soares <soares@synopsys.com> > >> > >> This patch slipts dw-i3c-master.c into three pieces: > >> dw-i3c-master.c - contains the code that interacts directly with the > >> core in master mode. > >> > >> dw-i3c-platdrv.c - contains the code specific to the platform driver. > >> > >> dw-i3c-core.h - contains the definitions and declarations shared by > >> dw-i3c-master and dw-i3c-platdrv > >> > >> This patch will allow SOC integrators to add their code specific to > >> DesignWare I3C IP. > > Isn't it too early to do this change? Can't we wait until we have a SoC > > that actually embeds this IP? > > > I'm trying to turn it more flexible so the other can reuse the code. Looking at the separation you've done here, I don't see why you need it. All the resources you request are generic, so why not just adding a new compat in the of_match_table? > > > > > >> Signed-off-by: Vitor Soares <soares@synopsys.com> > >> --- > >> drivers/i3c/master/Kconfig | 9 +- > >> drivers/i3c/master/Makefile | 5 +- > >> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ > >> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- > >> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ Just realized the driver is named dw-i3c-master, while the cadence driver is named i3c-master-cdns.c. I'll send a patch to make that consistent and follow the initial naming scheme: i3c-master-<ipname>.c.
Hi Boris, On 23/11/18 12:50, Boris Brezillon wrote: > On Fri, 23 Nov 2018 12:39:31 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> Hi Boris, >> >> >> On 22/11/18 20:02, Boris Brezillon wrote: >>> On Thu, 22 Nov 2018 17:54:54 +0000 >>> Vitor Soares <vitor.soares@synopsys.com> wrote: >>> >>>> From: Vitor Soares <soares@synopsys.com> >>>> >>>> This patch slipts dw-i3c-master.c into three pieces: >>>> dw-i3c-master.c - contains the code that interacts directly with the >>>> core in master mode. >>>> >>>> dw-i3c-platdrv.c - contains the code specific to the platform driver. >>>> >>>> dw-i3c-core.h - contains the definitions and declarations shared by >>>> dw-i3c-master and dw-i3c-platdrv >>>> >>>> This patch will allow SOC integrators to add their code specific to >>>> DesignWare I3C IP. >>> Isn't it too early to do this change? Can't we wait until we have a SoC >>> that actually embeds this IP? >> >> I'm trying to turn it more flexible so the other can reuse the code. > Looking at the separation you've done here, I don't see why you need > it. All the resources you request are generic, so why not just adding a > new compat in the of_match_table? I understand your point. I'm just following what it's done in others Synopsys drivers and what I expect is that in the future we will have the same for the I3C. Some of the current generic functions might be override according with SoC requirements (e.g i2c-designware, pcie-designware). for now what do you prefer? >> >>> >>>> Signed-off-by: Vitor Soares <soares@synopsys.com> >>>> --- >>>> drivers/i3c/master/Kconfig | 9 +- >>>> drivers/i3c/master/Makefile | 5 +- >>>> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ >>>> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- >>>> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ > Just realized the driver is named dw-i3c-master, while the cadence > driver is named i3c-master-cdns.c. I'll send a patch to make that > consistent and follow the initial naming scheme: i3c-master-<ipname>.c. As I shared with you in previous email, the structure that I have in mind is this one: - core.h (or common.h, any though?) - common.c - master.c - slave.c so for me doesn't make sense to have for instance: i3c-master-dw-slave.c But seeing what is already in the kernel I wasn't coherent and it should be named to i3c-designware-master.c or follow this https://lkml.org/lkml/2017/7/12/430 This topic rise another one related with the master folder. I understand that now the subsystem doesn't have slave support but the name is limited. Isn't better to have something like controller or busses? What do you have in mind for the slave? Best regards, Vitor Soares
On Mon, 26 Nov 2018 12:06:24 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris, > > > On 23/11/18 12:50, Boris Brezillon wrote: > > On Fri, 23 Nov 2018 12:39:31 +0000 > > vitor <vitor.soares@synopsys.com> wrote: > > > >> Hi Boris, > >> > >> > >> On 22/11/18 20:02, Boris Brezillon wrote: > >>> On Thu, 22 Nov 2018 17:54:54 +0000 > >>> Vitor Soares <vitor.soares@synopsys.com> wrote: > >>> > >>>> From: Vitor Soares <soares@synopsys.com> > >>>> > >>>> This patch slipts dw-i3c-master.c into three pieces: > >>>> dw-i3c-master.c - contains the code that interacts directly with the > >>>> core in master mode. > >>>> > >>>> dw-i3c-platdrv.c - contains the code specific to the platform driver. > >>>> > >>>> dw-i3c-core.h - contains the definitions and declarations shared by > >>>> dw-i3c-master and dw-i3c-platdrv > >>>> > >>>> This patch will allow SOC integrators to add their code specific to > >>>> DesignWare I3C IP. > >>> Isn't it too early to do this change? Can't we wait until we have a SoC > >>> that actually embeds this IP? > >> > >> I'm trying to turn it more flexible so the other can reuse the code. > > Looking at the separation you've done here, I don't see why you need > > it. All the resources you request are generic, so why not just adding a > > new compat in the of_match_table? > > I understand your point. > > > I'm just following what it's done in others Synopsys drivers and what I > expect is that in the future we will have the same for the I3C. > > Some of the current generic functions might be override according with > SoC requirements (e.g i2c-designware, pcie-designware). > > > for now what do you prefer? > I prefer that we keep the driver as is until we actually need to split things up. > >> > >>> > >>>> Signed-off-by: Vitor Soares <soares@synopsys.com> > >>>> --- > >>>> drivers/i3c/master/Kconfig | 9 +- > >>>> drivers/i3c/master/Makefile | 5 +- > >>>> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ > >>>> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- > >>>> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ > > Just realized the driver is named dw-i3c-master, while the cadence > > driver is named i3c-master-cdns.c. I'll send a patch to make that > > consistent and follow the initial naming scheme: i3c-master-<ipname>.c. > > As I shared with you in previous email, the structure that I have in > mind is this one: > > - core.h (or common.h, any though?) > > - common.c > > - master.c > > - slave.c > > > so for me doesn't make sense to have for instance: i3c-master-dw-slave.c If you have several files and they're all placed in a dw/ subdir, then I agree, prefixing everything with i3c-master- is useless, as you'll have to define a custom rule to create the i3c-master-dw.ko object. When there's a single source file, and this source file is directly used to create a .ko, we need this prefix, otherwise we would have dw.ko, and this would basically conflict with any other designware driver that does not have a proper prefix. > > But seeing what is already in the kernel I wasn't coherent and it should > be named to i3c-designware-master.c Actually it's i3c-master-designware.c (or i3c-master-dw.c) if we follow what's been done for the cadence driver. > > > or > > > follow this https://lkml.org/lkml/2017/7/12/430 And I agree with Linus on this, except that does not apply to single source file drivers. > > > This topic rise another one related with the master folder. I understand > that now the subsystem doesn't have slave support but the name is > limited. Isn't better to have something like controller or busses? What > do you have in mind for the slave? drivers/i3c/slave/... for slave drivers and drivers/i3c/slave.c for the framework, just like we have drivers/i3c/master/ for master controller drivers and drivers/i3c/master.c.
On 26/11/18 12:35, Boris Brezillon wrote: > On Mon, 26 Nov 2018 12:06:24 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> Hi Boris, >> >> >> On 23/11/18 12:50, Boris Brezillon wrote: >>> On Fri, 23 Nov 2018 12:39:31 +0000 >>> vitor <vitor.soares@synopsys.com> wrote: >>> >>>> Hi Boris, >>>> >>>> >>>> On 22/11/18 20:02, Boris Brezillon wrote: >>>>> On Thu, 22 Nov 2018 17:54:54 +0000 >>>>> Vitor Soares <vitor.soares@synopsys.com> wrote: >>>>> >>>>>> From: Vitor Soares <soares@synopsys.com> >>>>>> >>>>>> This patch slipts dw-i3c-master.c into three pieces: >>>>>> dw-i3c-master.c - contains the code that interacts directly with the >>>>>> core in master mode. >>>>>> >>>>>> dw-i3c-platdrv.c - contains the code specific to the platform driver. >>>>>> >>>>>> dw-i3c-core.h - contains the definitions and declarations shared by >>>>>> dw-i3c-master and dw-i3c-platdrv >>>>>> >>>>>> This patch will allow SOC integrators to add their code specific to >>>>>> DesignWare I3C IP. >>>>> Isn't it too early to do this change? Can't we wait until we have a SoC >>>>> that actually embeds this IP? >>>> I'm trying to turn it more flexible so the other can reuse the code. >>> Looking at the separation you've done here, I don't see why you need >>> it. All the resources you request are generic, so why not just adding a >>> new compat in the of_match_table? >> I understand your point. >> >> >> I'm just following what it's done in others Synopsys drivers and what I >> expect is that in the future we will have the same for the I3C. >> >> Some of the current generic functions might be override according with >> SoC requirements (e.g i2c-designware, pcie-designware). >> >> >> for now what do you prefer? >> > I prefer that we keep the driver as is until we actually need to split > things up. This is already done and will benefit everyone: - for me is better do it now than the secondary master and slave development. - for the others it will easy the SoC integration avoiding duplicated work and doing things from scratch. > >>>> >>>>> >>>>>> Signed-off-by: Vitor Soares <soares@synopsys.com> >>>>>> --- >>>>>> drivers/i3c/master/Kconfig | 9 +- >>>>>> drivers/i3c/master/Makefile | 5 +- >>>>>> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ >>>>>> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- >>>>>> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ >>> Just realized the driver is named dw-i3c-master, while the cadence >>> driver is named i3c-master-cdns.c. I'll send a patch to make that >>> consistent and follow the initial naming scheme: i3c-master-<ipname>.c. >> As I shared with you in previous email, the structure that I have in >> mind is this one: >> >> - core.h (or common.h, any though?) >> >> - common.c >> >> - master.c >> >> - slave.c >> >> >> so for me doesn't make sense to have for instance: i3c-master-dw-slave.c > If you have several files and they're all placed in a dw/ subdir, then > I agree, prefixing everything with i3c-master- is useless, as you'll > have to define a custom rule to create the i3c-master-dw.ko object. > > When there's a single source file, and this source file is directly > used to create a .ko, we need this prefix, otherwise we would have > dw.ko, and this would basically conflict with any other designware > driver that does not have a proper prefix. > >> But seeing what is already in the kernel I wasn't coherent and it should >> be named to i3c-designware-master.c > Actually it's i3c-master-designware.c (or i3c-master-dw.c) if we follow > what's been done for the cadence driver. > I was referring to what was made in other modules and should be applied here too. >> >> or >> >> >> follow this https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_7_12_430&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=9fGCPbkiqaG2-CJ5qrOU2Os6ZcstSNxi7UbQiF9YEBk&s=ADR3LotyBBy6e8Rv-UFW_-J8B5os_PY71QtUols3tb4&e= > And I agree with Linus on this, except that does not apply to single > source file drivers. > >> >> This topic rise another one related with the master folder. I understand >> that now the subsystem doesn't have slave support but the name is >> limited. Isn't better to have something like controller or busses? What >> do you have in mind for the slave? > drivers/i3c/slave/... for slave drivers and drivers/i3c/slave.c for the > framework, just like we have drivers/i3c/master/ for master controller > drivers and drivers/i3c/master.c. I have to disagree here. I don't see any place on the kernel with .../master/ and ../slave/ folders and it is very likely that both rules will have some common code. With this structure the user will have the code spread in /master and /slave folders... I would like you consider to change the folder name and the names rules to something like in i2c. Maybe someone else can give his opinion too. Best regards, Vitor Soares
On Mon, 26 Nov 2018 18:33:37 +0000 vitor <vitor.soares@synopsys.com> wrote: > On 26/11/18 12:35, Boris Brezillon wrote: > > On Mon, 26 Nov 2018 12:06:24 +0000 > > vitor <vitor.soares@synopsys.com> wrote: > > > >> Hi Boris, > >> > >> > >> On 23/11/18 12:50, Boris Brezillon wrote: > >>> On Fri, 23 Nov 2018 12:39:31 +0000 > >>> vitor <vitor.soares@synopsys.com> wrote: > >>> > >>>> Hi Boris, > >>>> > >>>> > >>>> On 22/11/18 20:02, Boris Brezillon wrote: > >>>>> On Thu, 22 Nov 2018 17:54:54 +0000 > >>>>> Vitor Soares <vitor.soares@synopsys.com> wrote: > >>>>> > >>>>>> From: Vitor Soares <soares@synopsys.com> > >>>>>> > >>>>>> This patch slipts dw-i3c-master.c into three pieces: > >>>>>> dw-i3c-master.c - contains the code that interacts directly with the > >>>>>> core in master mode. > >>>>>> > >>>>>> dw-i3c-platdrv.c - contains the code specific to the platform driver. > >>>>>> > >>>>>> dw-i3c-core.h - contains the definitions and declarations shared by > >>>>>> dw-i3c-master and dw-i3c-platdrv > >>>>>> > >>>>>> This patch will allow SOC integrators to add their code specific to > >>>>>> DesignWare I3C IP. > >>>>> Isn't it too early to do this change? Can't we wait until we have a SoC > >>>>> that actually embeds this IP? > >>>> I'm trying to turn it more flexible so the other can reuse the code. > >>> Looking at the separation you've done here, I don't see why you need > >>> it. All the resources you request are generic, so why not just adding a > >>> new compat in the of_match_table? > >> I understand your point. > >> > >> > >> I'm just following what it's done in others Synopsys drivers and what I > >> expect is that in the future we will have the same for the I3C. > >> > >> Some of the current generic functions might be override according with > >> SoC requirements (e.g i2c-designware, pcie-designware). > >> > >> > >> for now what do you prefer? > >> > > I prefer that we keep the driver as is until we actually need to split > > things up. > > This is already done and will benefit everyone: > > - for me is better do it now than the secondary master and slave > development. Sorry, I don't get that one. > > - for the others it will easy the SoC integration avoiding > duplicated work and doing things from scratch. What would be duplicated? You want to support a new SoC, just add a new entry in the of_match_table and you're done. When you need to add SoC/integration specific stuff, create a struct and attach a different instance per-compatible so that each SoC can have its own configuration (or even init sequence if needed). That's how we do for pretty much all IPs out there, why should designware ones be different? > > > > > >>>> > >>>>> > >>>>>> Signed-off-by: Vitor Soares <soares@synopsys.com> > >>>>>> --- > >>>>>> drivers/i3c/master/Kconfig | 9 +- > >>>>>> drivers/i3c/master/Makefile | 5 +- > >>>>>> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ > >>>>>> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- > >>>>>> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ > >>> Just realized the driver is named dw-i3c-master, while the cadence > >>> driver is named i3c-master-cdns.c. I'll send a patch to make that > >>> consistent and follow the initial naming scheme: i3c-master-<ipname>.c. > >> As I shared with you in previous email, the structure that I have in > >> mind is this one: > >> > >> - core.h (or common.h, any though?) > >> > >> - common.c > >> > >> - master.c > >> > >> - slave.c > >> > >> > >> so for me doesn't make sense to have for instance: i3c-master-dw-slave.c > > If you have several files and they're all placed in a dw/ subdir, then > > I agree, prefixing everything with i3c-master- is useless, as you'll > > have to define a custom rule to create the i3c-master-dw.ko object. > > > > When there's a single source file, and this source file is directly > > used to create a .ko, we need this prefix, otherwise we would have > > dw.ko, and this would basically conflict with any other designware > > driver that does not have a proper prefix. > > > >> But seeing what is already in the kernel I wasn't coherent and it should > >> be named to i3c-designware-master.c > > Actually it's i3c-master-designware.c (or i3c-master-dw.c) if we follow > > what's been done for the cadence driver. > > > > I was referring to what was made in other modules and should be applied > here too. This is a subsystem decision. I don't mind changing the naming scheme, though I don't see why yours is better than the one I initially proposed. In any case, what's important here is to keep driver names consistent. > > > >> > >> or > >> > >> > >> follow this https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_7_12_430&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=9fGCPbkiqaG2-CJ5qrOU2Os6ZcstSNxi7UbQiF9YEBk&s=ADR3LotyBBy6e8Rv-UFW_-J8B5os_PY71QtUols3tb4&e= > > And I agree with Linus on this, except that does not apply to single > > source file drivers. > > > >> > >> This topic rise another one related with the master folder. I understand > >> that now the subsystem doesn't have slave support but the name is > >> limited. Isn't better to have something like controller or busses? What > >> do you have in mind for the slave? > > drivers/i3c/slave/... for slave drivers and drivers/i3c/slave.c for the > > framework, just like we have drivers/i3c/master/ for master controller > > drivers and drivers/i3c/master.c. > > I have to disagree here. I don't see any place on the kernel with > .../master/ and ../slave/ folders and it is very likely that both rules > will have some common code. I see at least one that uses this model: the USB framework (drivers/usb/gadget/ for device controllers and drivers/usb/host/ for host controllers). Given that I3C is closer to USB than I2C I initially decided to keep this separation. Maybe I'm wrong, but I'd like to understand why you think it's not appropriate. > > With this structure the user will have the code spread in /master and > /slave folders... Not sure who you call "user" here, but yes, master controller and slave controller drivers would be placed in different dirs. > > > I would like you consider to change the folder name and the names rules > to something like in i2c. Why? And more importantly, why is this coming up now? You've been reviewing the framework since the beginning, and never complained about the subdirs/files organization so far. I'm okay changing it, but I want to understand why the proposed separation is not good.
On Mon, 26 Nov 2018 19:56:18 +0100 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > > - for the others it will easy the SoC integration avoiding > > duplicated work and doing things from scratch. > > What would be duplicated? You want to support a new SoC, just add a new > entry in the of_match_table and you're done. When you need to add > SoC/integration specific stuff, create a struct and attach a different > instance per-compatible so that each SoC can have its own configuration > (or even init sequence if needed). That's how we do for pretty much all > IPs out there, why should designware ones be different? To be more specific, I'd like a real example that shows why the separation is needed.
On 26/11/18 19:08, Boris Brezillon wrote: > On Mon, 26 Nov 2018 19:56:18 +0100 > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > >>> - for the others it will easy the SoC integration avoiding >>> duplicated work and doing things from scratch. >> What would be duplicated? You want to support a new SoC, just add a new >> entry in the of_match_table and you're done. When you need to add >> SoC/integration specific stuff, create a struct and attach a different >> instance per-compatible so that each SoC can have its own configuration >> (or even init sequence if needed). That's how we do for pretty much all >> IPs out there, why should designware ones be different? > To be more specific, I'd like a real example that shows why the > separation is needed. Ok no problem. We can delay this for PCI and other rules support.
On Mon, 26 Nov 2018 19:28:02 +0000 vitor <vitor.soares@synopsys.com> wrote: > On 26/11/18 19:08, Boris Brezillon wrote: > > On Mon, 26 Nov 2018 19:56:18 +0100 > > Boris Brezillon <boris.brezillon@bootlin.com> wrote: > > > >>> - for the others it will easy the SoC integration avoiding > >>> duplicated work and doing things from scratch. > >> What would be duplicated? You want to support a new SoC, just add a new > >> entry in the of_match_table and you're done. When you need to add > >> SoC/integration specific stuff, create a struct and attach a different > >> instance per-compatible so that each SoC can have its own configuration > >> (or even init sequence if needed). That's how we do for pretty much all > >> IPs out there, why should designware ones be different? > > To be more specific, I'd like a real example that shows why the > > separation is needed. > > Ok no problem. We can delay this for PCI and other rules support. I finally understand what this separation is all about: supporting both PCI and platform devices. I guess I've been distracted by this sentence: " This patch will allow SOC integrators to add their code specific to DesignWare I3C IP. " which for me meant each SoC would have its own platform_driver. In any case, I think this is a bit premature do this separation, unless you already know about one integrator planning to expose this IP over PCI.
On 26/11/18 18:56, Boris Brezillon wrote: > On Mon, 26 Nov 2018 18:33:37 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> On 26/11/18 12:35, Boris Brezillon wrote: >>> On Mon, 26 Nov 2018 12:06:24 +0000 >>> vitor <vitor.soares@synopsys.com> wrote: >>> >>>> Hi Boris, >>>> >>>> >>>> On 23/11/18 12:50, Boris Brezillon wrote: >>>>> On Fri, 23 Nov 2018 12:39:31 +0000 >>>>> vitor <vitor.soares@synopsys.com> wrote: >>>>> >>>>>> Hi Boris, >>>>>> >>>>>> >>>>>> On 22/11/18 20:02, Boris Brezillon wrote: >>>>>>> On Thu, 22 Nov 2018 17:54:54 +0000 >>>>>>> Vitor Soares <vitor.soares@synopsys.com> wrote: >>>>>>> >>>>>>>> From: Vitor Soares <soares@synopsys.com> >>>>>>>> >>>>>>>> This patch slipts dw-i3c-master.c into three pieces: >>>>>>>> dw-i3c-master.c - contains the code that interacts directly with the >>>>>>>> core in master mode. >>>>>>>> >>>>>>>> dw-i3c-platdrv.c - contains the code specific to the platform driver. >>>>>>>> >>>>>>>> dw-i3c-core.h - contains the definitions and declarations shared by >>>>>>>> dw-i3c-master and dw-i3c-platdrv >>>>>>>> >>>>>>>> This patch will allow SOC integrators to add their code specific to >>>>>>>> DesignWare I3C IP. >>>>>>> Isn't it too early to do this change? Can't we wait until we have a SoC >>>>>>> that actually embeds this IP? >>>>>> I'm trying to turn it more flexible so the other can reuse the code. >>>>> Looking at the separation you've done here, I don't see why you need >>>>> it. All the resources you request are generic, so why not just adding a >>>>> new compat in the of_match_table? >>>> I understand your point. >>>> >>>> >>>> I'm just following what it's done in others Synopsys drivers and what I >>>> expect is that in the future we will have the same for the I3C. >>>> >>>> Some of the current generic functions might be override according with >>>> SoC requirements (e.g i2c-designware, pcie-designware). >>>> >>>> >>>> for now what do you prefer? >>>> >>> I prefer that we keep the driver as is until we actually need to split >>> things up. >> This is already done and will benefit everyone: >> >> - for me is better do it now than the secondary master and slave >> development. > Sorry, I don't get that one. I already share my plan with you. See the structure above. > >> - for the others it will easy the SoC integration avoiding >> duplicated work and doing things from scratch. > What would be duplicated? You want to support a new SoC, just add a new > entry in the of_match_table and you're done. When you need to add > SoC/integration specific stuff, create a struct and attach a different > instance per-compatible so that each SoC can have its own configuration > (or even init sequence if needed). That's how we do for pretty much all > IPs out there, why should designware ones be different? > >> >>> >>>>>> >>>>>>> >>>>>>>> Signed-off-by: Vitor Soares <soares@synopsys.com> >>>>>>>> --- >>>>>>>> drivers/i3c/master/Kconfig | 9 +- >>>>>>>> drivers/i3c/master/Makefile | 5 +- >>>>>>>> drivers/i3c/master/dw-i3c-core.h | 214 ++++++++++++++++++++++++++ >>>>>>>> drivers/i3c/master/dw-i3c-master.c | 299 ++---------------------------------- >>>>>>>> drivers/i3c/master/dw-i3c-platdrv.c | 112 ++++++++++++++ >>>>> Just realized the driver is named dw-i3c-master, while the cadence >>>>> driver is named i3c-master-cdns.c. I'll send a patch to make that >>>>> consistent and follow the initial naming scheme: i3c-master-<ipname>.c. >>>> As I shared with you in previous email, the structure that I have in >>>> mind is this one: >>>> >>>> - core.h (or common.h, any though?) >>>> >>>> - common.c >>>> >>>> - master.c >>>> >>>> - slave.c >>>> >>>> >>>> so for me doesn't make sense to have for instance: i3c-master-dw-slave.c >>> If you have several files and they're all placed in a dw/ subdir, then >>> I agree, prefixing everything with i3c-master- is useless, as you'll >>> have to define a custom rule to create the i3c-master-dw.ko object. >>> >>> When there's a single source file, and this source file is directly >>> used to create a .ko, we need this prefix, otherwise we would have >>> dw.ko, and this would basically conflict with any other designware >>> driver that does not have a proper prefix. >>> >>>> But seeing what is already in the kernel I wasn't coherent and it should >>>> be named to i3c-designware-master.c >>> Actually it's i3c-master-designware.c (or i3c-master-dw.c) if we follow >>> what's been done for the cadence driver. >>> >> I was referring to what was made in other modules and should be applied >> here too. > This is a subsystem decision. I don't mind changing the naming scheme, > though I don't see why yours is better than the one I initially > proposed. In any case, what's important here is to keep driver names > consistent. > >> >>>> or >>>> >>>> >>>> follow this https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_7_12_430&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=9fGCPbkiqaG2-CJ5qrOU2Os6ZcstSNxi7UbQiF9YEBk&s=ADR3LotyBBy6e8Rv-UFW_-J8B5os_PY71QtUols3tb4&e= >>> And I agree with Linus on this, except that does not apply to single >>> source file drivers. >>> >>>> This topic rise another one related with the master folder. I understand >>>> that now the subsystem doesn't have slave support but the name is >>>> limited. Isn't better to have something like controller or busses? What >>>> do you have in mind for the slave? >>> drivers/i3c/slave/... for slave drivers and drivers/i3c/slave.c for the >>> framework, just like we have drivers/i3c/master/ for master controller >>> drivers and drivers/i3c/master.c. >> I have to disagree here. I don't see any place on the kernel with >> .../master/ and ../slave/ folders and it is very likely that both rules >> will have some common code. > I see at least one that uses this model: the USB framework > (drivers/usb/gadget/ for device controllers and drivers/usb/host/ for > host controllers). Given that I3C is closer to USB than I2C I initially > decided to keep this separation. Maybe I'm wrong, but I'd like to > understand why you think it's not appropriate. You can say there some features from USB in I3C but cannot compare USB vs I3C since they are in different championships. The aim of I3C is to fill the gaps discovery on I2C over the years but still keeping its simplicity not to go to the complexity of USB. I'm not sure but I think that a controller cannot change between gadget to host in USB in runtime. Even so, this kind of behavior is more likely to have in an I3C bus. > >> With this structure the user will have the code spread in /master and >> /slave folders... > Not sure who you call "user" here, but yes, master controller and slave > controller drivers would be placed in different dirs. > >> >> I would like you consider to change the folder name and the names rules >> to something like in i2c. > Why? And more importantly, why is this coming up now? You've been > reviewing the framework since the beginning, and never complained about > the subdirs/files organization so far. Sorry for that and don't take me wrong... maybe I should rise this question early but this only came up now when I started splitting and thinking where to put what is for master for slave, what is common and the thing of putting everything of controller in a folder. Taking the USB as exemple do you prefer a dwc folder on i3c root? > > I'm okay changing it, but I want to understand why the proposed > separation is not good. I already tell you my use case and as I said maybe someone can advise :)
On Mon, 26 Nov 2018 20:11:39 +0000 vitor <vitor.soares@synopsys.com> wrote: > >>> I prefer that we keep the driver as is until we actually need to split > >>> things up. > >> This is already done and will benefit everyone: > >> > >> - for me is better do it now than the secondary master and slave > >> development. > > Sorry, I don't get that one. > > > I already share my plan with you. See the structure above. My point is, I don't get the relationship between your patch and secondary-master/slave-mode support. > >>>> This topic rise another one related with the master folder. I understand > >>>> that now the subsystem doesn't have slave support but the name is > >>>> limited. Isn't better to have something like controller or busses? What > >>>> do you have in mind for the slave? > >>> drivers/i3c/slave/... for slave drivers and drivers/i3c/slave.c for the > >>> framework, just like we have drivers/i3c/master/ for master controller > >>> drivers and drivers/i3c/master.c. > >> I have to disagree here. I don't see any place on the kernel with > >> .../master/ and ../slave/ folders and it is very likely that both rules > >> will have some common code. > > I see at least one that uses this model: the USB framework > > (drivers/usb/gadget/ for device controllers and drivers/usb/host/ for > > host controllers). Given that I3C is closer to USB than I2C I initially > > decided to keep this separation. Maybe I'm wrong, but I'd like to > > understand why you think it's not appropriate. > > > You can say there some features from USB in I3C but cannot compare USB > vs I3C since they are in different championships. I maintain that functionally, I3C is closer to USB than I2C. That does not mean that it's competing with USB performance-wise, it just means that the SW stack is likely to resemble the USB one (probably a bit simpler, at least at the beginning). > > The aim of I3C is to fill the gaps discovery on I2C over the years but > still keeping its simplicity not to go to the complexity of USB. Look at the bus discovery mechanism, the notion of DCR (close to the concept of USB class), or the fact that each dev has a unique manufacturer+PID pair (which resemble the product/vendor ID concept) that allows us to easily bind a dev to a driver without requiring a static description. > > > I'm not sure but I think that a controller cannot change between gadget > to host in USB in runtime. That's called USB OTG. Okay, to be fair, it's not exactly the same, and the mastership handover in I3C is probably more complex than what we have with USB OTG (I'm not a USB expert, so I might be wrong here). > Even so, this kind of behavior is more likely > to have in an I3C bus. Maybe. > > > > > >> With this structure the user will have the code spread in /master and > >> /slave folders... > > Not sure who you call "user" here, but yes, master controller and slave > > controller drivers would be placed in different dirs. > > > >> > >> I would like you consider to change the folder name and the names rules > >> to something like in i2c. > > Why? And more importantly, why is this coming up now? You've been > > reviewing the framework since the beginning, and never complained about > > the subdirs/files organization so far. > > > Sorry for that and don't take me wrong... maybe I should rise this > question early but this only came up now when I started splitting and > thinking where to put what is for master for slave, what is common and > the thing of putting everything of controller in a folder. So you have such a dual-role controller? I mean, Cadence IP has a dummy GPIO mode in its Master IP when is operating in slave mode (secondary master, or main master after it's released the bus), but I'm not sure this was designed for anything else but testing. What I call a slave controller would be something that lets you reply to SDR/DDR transactions or fill a generic regmap that would be exposed to other masters on the bus. This way we could implement generic slave drivers in Linux (the same way we have gadget drivers). Anything else is likely to be to specific to be exposed as a generic framework. > > > Taking the USB as exemple do you prefer a dwc folder on i3c root? Hm, not sure I like this idea either. So I see 2 options: 1/ put all controller drivers (both master and slave ones) in a common directory (drivers/i3c/controllers) as you suggest, and prefix them correctly (i3c-master-<ip>.c, i3c-slave-<ip>.c and i3c-dual-<ip>.c) 2/ place them in separate directories: drivers/i3c/{master,slave,dual} I'm fine either way. > > > > > > I'm okay changing it, but I want to understand why the proposed > > separation is not good. > > > I already tell you my use case and as I said maybe someone can advise :) > I think I understand your concerns now, but only because you started to mention a few things that were not clearly stated before (at least, I didn't understand it this way), like the fact that your controller (and probably others too) supports dual-role, or the fact that you plan to expose your IP through the PCI bus.
Hi Boris On 26/11/18 21:33, Boris Brezillon wrote: > On Mon, 26 Nov 2018 20:11:39 +0000 > vitor <vitor.soares@synopsys.com> wrote: > > Look at the bus discovery mechanism, the notion of DCR (close to the > concept of USB class), or the fact that each dev has a unique > manufacturer+PID pair (which resemble the product/vendor ID concept) > that allows us to easily bind a dev to a driver without requiring a > static description. The major feature close to USB is this one and it can be found in others protocols (standardization process). Just to close this topic I3C vs USB, IMO it's wrong to pass the message that the I3C is closer to USB than I2C even more because I3C support the I2C on the fly. > >> >> I'm not sure but I think that a controller cannot change between gadget >> to host in USB in runtime. > That's called USB OTG. Okay, to be fair, it's not exactly the same, and > the mastership handover in I3C is probably more complex than what we > have with USB OTG (I'm not a USB expert, so I might be wrong here). > >> Even so, this kind of behavior is more likely >> to have in an I3C bus. > Maybe. Sorry, with the proliferation of sensors I cannot see a multi master sensor network based on USB. >> Sorry for that and don't take me wrong... maybe I should rise this >> question early but this only came up now when I started splitting and >> thinking where to put what is for master for slave, what is common and >> the thing of putting everything of controller in a folder. > So you have such a dual-role controller? Yes, we already talked about secondary master support. > What I call a slave controller would be something that lets you reply to > SDR/DDR transactions or fill a generic regmap that would be exposed to > other masters on the bus. This way we could implement generic slave > drivers in Linux (the same way we have gadget drivers). Anything else > is likely to be to specific to be exposed as a generic framework. I would bet to do something like in i2c, we don't need the same level of complexity found in USB. >> >> Taking the USB as exemple do you prefer a dwc folder on i3c root? > Hm, not sure I like this idea either. So I see 2 options: > > 1/ put all controller drivers (both master and slave ones) in a common > directory (drivers/i3c/controllers) as you suggest, and prefix them > correctly (i3c-master-<ip>.c, i3c-slave-<ip>.c and i3c-dual-<ip>.c) I agree with the controller folder but not with prefix. Please check what is already in the kernel. > 2/ place them in separate directories: drivers/i3c/{master,slave,dual} > > I'm fine either way. In this case and taking what is already in the kernel it will be drivers/i3c/{master, slave, dwc, other with the same architecture as dwc}. >>> I'm okay changing it, but I want to understand why the proposed >>> separation is not good. >> >> I already tell you my use case and as I said maybe someone can advise :) >> > I think I understand your concerns now, but only because you started to > mention a few things that were not clearly stated before (at least, I > didn't understand it this way), like the fact that your controller (and > probably others too) supports dual-role, or the fact that you plan to > expose your IP through the PCI bus. I miss to mention PCI but since the beginning refer the slave and the common part. Splitting the driver is something that soon or later I will have to do. If you prefer later I'm ok with that. I think this discussion is starting to be counterproductive with arguing of both parts. Unfortunately I don't see anyone given their inputs too. To be clear, the subsystem is nice and I working with daily. As I said this is something that I dealing now and I'm telling what I think that is not correct.
Hi Vitor, On Tue, 27 Nov 2018 11:50:53 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris > > On 26/11/18 21:33, Boris Brezillon wrote: > > On Mon, 26 Nov 2018 20:11:39 +0000 > > vitor <vitor.soares@synopsys.com> wrote: > > > > Look at the bus discovery mechanism, the notion of DCR (close to the > > concept of USB class), or the fact that each dev has a unique > > manufacturer+PID pair (which resemble the product/vendor ID concept) > > that allows us to easily bind a dev to a driver without requiring a > > static description. > > > The major feature close to USB is this one and it can be found in others > protocols (standardization process). > > Just to close this topic I3C vs USB, IMO it's wrong to pass the message > that the I3C is closer to USB than I2C even more because I3C support the > I2C on the fly. I think you didn't read my reply carefully. I'm not saying I3C == USB, I'm just saying that the way you interact with an I3C from a SW PoV is not at all the same as you would do for an I2C device. Do you deny that? > > > > > >> > >> I'm not sure but I think that a controller cannot change between gadget > >> to host in USB in runtime. > > That's called USB OTG. Okay, to be fair, it's not exactly the same, and > > the mastership handover in I3C is probably more complex than what we > > have with USB OTG (I'm not a USB expert, so I might be wrong here). > > > >> Even so, this kind of behavior is more likely > >> to have in an I3C bus. > > Maybe. > > > Sorry, with the proliferation of sensors I cannot see a multi master > sensor network based on USB. Looks like there's a misunderstanding here. The question is not whether I3C will replace I2C or USB, of course it's meant to overcome the limitations of I2C. I'm just pointing out that, if we have to expose I3C devices, we should look at what other discoverable buses do (PCI, USB, ...), not what I2C does. > > > >> Sorry for that and don't take me wrong... maybe I should rise this > >> question early but this only came up now when I started splitting and > >> thinking where to put what is for master for slave, what is common and > >> the thing of putting everything of controller in a folder. > > So you have such a dual-role controller? > > > Yes, we already talked about secondary master support. There's a difference between a secondary master that waits for its time to become the currrent master, and a secondary master that provides I3C device features when it's acting as a slave (sensor, GPIO controller, ...). So far we focused on supporting the former. If there's a need for the latter, then we should start thinking about the slave framework... > > > > What I call a slave controller would be something that lets you reply to > > SDR/DDR transactions or fill a generic regmap that would be exposed to > > other masters on the bus. This way we could implement generic slave > > drivers in Linux (the same way we have gadget drivers). Anything else > > is likely to be to specific to be exposed as a generic framework. > > > I would bet to do something like in i2c, we don't need the same level of > complexity found in USB. Can you detail a bit more what you have in mind? I don't think we can do like I2C, simply because we need to expose a valid DCR + manuf-ID/PID so that other masters can bind the device to the appropriate driver on their side. Plus, if we're about to expose generic profiles, we likely don't want each I3C slave controller driver to do that on its own. > > > >> > >> Taking the USB as exemple do you prefer a dwc folder on i3c root? > > Hm, not sure I like this idea either. So I see 2 options: > > > > 1/ put all controller drivers (both master and slave ones) in a common > > directory (drivers/i3c/controllers) as you suggest, and prefix them > > correctly (i3c-master-<ip>.c, i3c-slave-<ip>.c and i3c-dual-<ip>.c) > > > I agree with the controller folder but not with prefix. Please check > what is already in the kernel. If we mix everything in the same subdir, I'd like to have an easy way to quickly identify those that are slave controllers and those that are master controllers. For the dual-role thing, maybe we can consider them as master (ones with advances slave features). Would you be okay with drivers/i3c/controllers/{designware,dw}/..., so that you can have all designware drivers (for both slave and master blocks) in the same dir? For those that are placed directly under drivers/i3c/controllers/... (because they only have one .c file), I'd like to keep a standard prefix. > > > > 2/ place them in separate directories: drivers/i3c/{master,slave,dual} > > > > I'm fine either way. > > > In this case and taking what is already in the kernel it will be > drivers/i3c/{master, slave, dwc, other with the same architecture as dwc}. And again, I'm questioning the necessity of per-IP directories at the root level. I'm not against per-IP directories, as long as they are classified like other HW blocks: drivers/i3c/{master,slave}/<ip>/... > > > >>> I'm okay changing it, but I want to understand why the proposed > >>> separation is not good. > >> > >> I already tell you my use case and as I said maybe someone can advise :) > >> > > I think I understand your concerns now, but only because you started to > > mention a few things that were not clearly stated before (at least, I > > didn't understand it this way), like the fact that your controller (and > > probably others too) supports dual-role, or the fact that you plan to > > expose your IP through the PCI bus. > > > I miss to mention PCI but since the beginning refer the slave and the > common part. > > Splitting the driver is something that soon or later I will have to do. > If you prefer later I'm ok with that. > > > I think this discussion is starting to be counterproductive with arguing > of both parts. No it's not vain, it's how we do discuss things in the community. I'm not saying I'm always right, but I need to understand the problems you're trying to solve to take a decision, and I don't think you initially gave all the details I needed to understand your PoV. That's a bit clearer now, even if I still disagree on a few aspects. > Unfortunately I don't see anyone given their inputs too. They will come. > > To be clear, the subsystem is nice and I working with daily. As I said > this is something that I dealing now and I'm telling what I think that > is not correct. > Come on! All I've seen so far are complaints on tiny details, it definitely doesn't prevent you from adding new features. Regards, Boris
Hi Boris, Sorry for the delayed response. On 27/11/18 12:33, Boris Brezillon wrote: > Hi Vitor, > > On Tue, 27 Nov 2018 11:50:53 +0000 > vitor<vitor.soares@synopsys.com> wrote: > > >>>> Sorry for that and don't take me wrong... maybe I should rise this >>>> question early but this only came up now when I started splitting and >>>> thinking where to put what is for master for slave, what is common and >>>> the thing of putting everything of controller in a folder. >>> So you have such a dual-role controller? >> Yes, we already talked about secondary master support. > There's a difference between a secondary master that waits for its time > to become the currrent master, and a secondary master that provides I3C > device features when it's acting as a slave (sensor, GPIO > controller, ...). So far we focused on supporting the former. If > there's a need for the latter, then we should start thinking about the > slave framework... > >>> What I call a slave controller would be something that lets you reply to >>> SDR/DDR transactions or fill a generic regmap that would be exposed to >>> other masters on the bus. This way we could implement generic slave >>> drivers in Linux (the same way we have gadget drivers). Anything else >>> is likely to be to specific to be exposed as a generic framework. >> I would bet to do something like in i2c, we don't need the same level of >> complexity found in USB. > Can you detail a bit more what you have in mind? I don't think we can > do like I2C, simply because we need to expose a valid DCR + > manuf-ID/PID so that other masters can bind the device to the > appropriate driver on their side. Plus, if we're about to expose > generic profiles, we likely don't want each I3C slave controller driver > to do that on its own. I think this should be discuss in another thread. Do you agree? >>>> Taking the USB as exemple do you prefer a dwc folder on i3c root? >>> Hm, not sure I like this idea either. So I see 2 options: >>> >>> 1/ put all controller drivers (both master and slave ones) in a common >>> directory (drivers/i3c/controllers) as you suggest, and prefix them >>> correctly (i3c-master-<ip>.c, i3c-slave-<ip>.c and i3c-dual-<ip>.c) >> I agree with the controller folder but not with prefix. Please check >> what is already in the kernel. > If we mix everything in the same subdir, I'd like to have an easy way > to quickly identify those that are slave controllers and those that are > master controllers. For the dual-role thing, maybe we can consider them > as master (ones with advances slave features). > > Would you be okay with drivers/i3c/controllers/{designware,dw}/..., so > that you can have all designware drivers (for both slave and master > blocks) in the same dir? Yes, that was what I trying to tell you. For me this might be the best option. I would like to avoid having dual role i3c driver in a master folder. > For those that are placed directly under drivers/i3c/controllers/... > (because they only have one .c file), I'd like to keep a standard > prefix. I don't disagree, and for those that have more than one file they should be in a folder, right? What prefix do you have in mind for those files inside a folder? >> To be clear, the subsystem is nice and I working with daily. As I said >> this is something that I dealing now and I'm telling what I think that >> is not correct. >> > Come on! All I've seen so far are complaints on tiny details, it > definitely doesn't prevent you from adding new features. > > Regards, > > Boris No, it's not. But as you can see to slipt the driver in parts this subject has some relevance. Best regards, Vitor Soares
On Tue, 4 Dec 2018 00:34:20 +0000 vitor <vitor.soares@synopsys.com> wrote: > >>> What I call a slave controller would be something that lets you reply to > >>> SDR/DDR transactions or fill a generic regmap that would be exposed to > >>> other masters on the bus. This way we could implement generic slave > >>> drivers in Linux (the same way we have gadget drivers). Anything else > >>> is likely to be to specific to be exposed as a generic framework. > >> I would bet to do something like in i2c, we don't need the same level of > >> complexity found in USB. > > Can you detail a bit more what you have in mind? I don't think we can > > do like I2C, simply because we need to expose a valid DCR + > > manuf-ID/PID so that other masters can bind the device to the > > appropriate driver on their side. Plus, if we're about to expose > > generic profiles, we likely don't want each I3C slave controller driver > > to do that on its own. > > > I think this should be discuss in another thread. Do you agree? If you want. Actually that's the most interesting part for me: discussing how we want to support I3C slave controllers or mixed master/slave controllers. All the driver split we're talking about here is just bikeshedding. > > > >>>> Taking the USB as exemple do you prefer a dwc folder on i3c root? > >>> Hm, not sure I like this idea either. So I see 2 options: > >>> > >>> 1/ put all controller drivers (both master and slave ones) in a common > >>> directory (drivers/i3c/controllers) as you suggest, and prefix them > >>> correctly (i3c-master-<ip>.c, i3c-slave-<ip>.c and i3c-dual-<ip>.c) > >> I agree with the controller folder but not with prefix. Please check > >> what is already in the kernel. > > If we mix everything in the same subdir, I'd like to have an easy way > > to quickly identify those that are slave controllers and those that are > > master controllers. For the dual-role thing, maybe we can consider them > > as master (ones with advances slave features). > > > > Would you be okay with drivers/i3c/controllers/{designware,dw}/..., so > > that you can have all designware drivers (for both slave and master > > blocks) in the same dir? > > > Yes, that was what I trying to tell you. For me this might be the best > option. Ok. > > I would like to avoid having dual role i3c driver in a master folder. I don't see why. If the driver is simple enough to fit in one file, there's no reason to create a new subdir. You think your DW IP is so complex and configurable that it requires several source files, fine, but please don't force others to do the same. > > > > For those that are placed directly under drivers/i3c/controllers/... > > (because they only have one .c file), I'd like to keep a standard > > prefix. > > > I don't disagree, and for those that have more than one file they should > be in a folder, right? Yes. > > What prefix do you have in mind for those files inside a folder? You mean, inside a sub-folder (drivers/i3c/controllers/{vendor}/)? It depends what you do with those source files. If they are to be exposed directly as modules, then they should be prefixed (i3c-<role>-<vendor>.c). On the other hand, if you create a single module out of several source files, source files don't need to be prefixed, as long as the resulting module as a proper prefix. > > > >> To be clear, the subsystem is nice and I working with daily. As I said > >> this is something that I dealing now and I'm telling what I think that > >> is not correct. > >> > > Come on! All I've seen so far are complaints on tiny details, it > > definitely doesn't prevent you from adding new features. > > > > Regards, > > > > Boris > > > No, it's not. But as you can see to slipt the driver in parts this > subject has some relevance. I'm not saying the discussion is useless, just that it's happening way too early compared to the other things we should work on. If you were adding support for slaves, and were doing this split as part of this patch series explaining that part of the code between slave and master can be shared, then we wouldn't have this debate. But right now, you're telling me that we need to split the DW driver to prepare for features that have not even been discussed/proposed. That's what I'm complaining about.
diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig index 8ee1ce6..fdc6e46 100644 --- a/drivers/i3c/master/Kconfig +++ b/drivers/i3c/master/Kconfig @@ -5,9 +5,14 @@ config CDNS_I3C_MASTER help Enable this driver if you want to support Cadence I3C master block. -config DW_I3C_MASTER - tristate "Synospsys DesignWare I3C master driver" +config DW_I3C_CORE + tristate + +config DW_I3C_PLATFORM + tristate "Synospsys DesignWare I3C Platform driver" + select DW_I3C_CORE depends on I3C + depends on HAS_IOMEM depends on !(ALPHA || PARISC) # ALPHA and PARISC needs {read,write}sl() help diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile index fc53939..004ad1c 100644 --- a/drivers/i3c/master/Makefile +++ b/drivers/i3c/master/Makefile @@ -1,2 +1,5 @@ obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o -obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o +obj-$(CONFIG_DW_I3C_CORE) += dw-i3c-core.o +dw-i3c-core-objs := dw-i3c-master.o +obj-$(CONFIG_DW_I3C_PLATFORM) += dw-i3c-platform.o +dw-i3c-platform-objs := dw-i3c-platdrv.o \ No newline at end of file diff --git a/drivers/i3c/master/dw-i3c-core.h b/drivers/i3c/master/dw-i3c-core.h new file mode 100644 index 0000000..277f63d --- /dev/null +++ b/drivers/i3c/master/dw-i3c-core.h @@ -0,0 +1,214 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates. + * + * Author: Vitor Soares <vitor.soares@synopsys.com> + */ + +#include <linux/i3c/master.h> +#include <linux/list.h> +#include <linux/clk.h> +#include <linux/completion.h> +#include <linux/reset.h> + +#define DEVICE_CTRL 0x0 +#define DEV_CTRL_ENABLE BIT(31) +#define DEV_CTRL_RESUME BIT(30) +#define DEV_CTRL_HOT_JOIN_NACK BIT(8) +#define DEV_CTRL_I2C_SLAVE_PRESENT BIT(7) + +#define DEVICE_ADDR 0x4 +#define DEV_ADDR_DYNAMIC_ADDR_VALID BIT(31) +#define DEV_ADDR_DYNAMIC(x) (((x) << 16) & GENMASK(22, 16)) + +#define HW_CAPABILITY 0x8 +#define COMMAND_QUEUE_PORT 0xc +#define COMMAND_PORT_TOC BIT(30) +#define COMMAND_PORT_READ_TRANSFER BIT(28) +#define COMMAND_PORT_SDAP BIT(27) +#define COMMAND_PORT_ROC BIT(26) +#define COMMAND_PORT_SPEED(x) (((x) << 21) & GENMASK(23, 21)) +#define COMMAND_PORT_DEV_INDEX(x) (((x) << 16) & GENMASK(20, 16)) +#define COMMAND_PORT_CP BIT(15) +#define COMMAND_PORT_CMD(x) (((x) << 7) & GENMASK(14, 7)) +#define COMMAND_PORT_TID(x) (((x) << 3) & GENMASK(6, 3)) + +#define COMMAND_PORT_ARG_DATA_LEN(x) (((x) << 16) & GENMASK(31, 16)) +#define COMMAND_PORT_ARG_DATA_LEN_MAX 65536 +#define COMMAND_PORT_TRANSFER_ARG 0x01 + +#define COMMAND_PORT_SDA_DATA_BYTE_3(x) (((x) << 24) & GENMASK(31, 24)) +#define COMMAND_PORT_SDA_DATA_BYTE_2(x) (((x) << 16) & GENMASK(23, 16)) +#define COMMAND_PORT_SDA_DATA_BYTE_1(x) (((x) << 8) & GENMASK(15, 8)) +#define COMMAND_PORT_SDA_BYTE_STRB_3 BIT(5) +#define COMMAND_PORT_SDA_BYTE_STRB_2 BIT(4) +#define COMMAND_PORT_SDA_BYTE_STRB_1 BIT(3) +#define COMMAND_PORT_SHORT_DATA_ARG 0x02 + +#define COMMAND_PORT_DEV_COUNT(x) (((x) << 21) & GENMASK(25, 21)) +#define COMMAND_PORT_ADDR_ASSGN_CMD 0x03 + +#define RESPONSE_QUEUE_PORT 0x10 +#define RESPONSE_PORT_ERR_STATUS(x) (((x) & GENMASK(31, 28)) >> 28) +#define RESPONSE_NO_ERROR 0 +#define RESPONSE_ERROR_CRC 1 +#define RESPONSE_ERROR_PARITY 2 +#define RESPONSE_ERROR_FRAME 3 +#define RESPONSE_ERROR_IBA_NACK 4 +#define RESPONSE_ERROR_ADDRESS_NACK 5 +#define RESPONSE_ERROR_OVER_UNDER_FLOW 6 +#define RESPONSE_ERROR_TRANSF_ABORT 8 +#define RESPONSE_ERROR_I2C_W_NACK_ERR 9 +#define RESPONSE_PORT_TID(x) (((x) & GENMASK(27, 24)) >> 24) +#define RESPONSE_PORT_DATA_LEN(x) ((x) & GENMASK(15, 0)) + +#define RX_TX_DATA_PORT 0x14 +#define IBI_QUEUE_STATUS 0x18 +#define QUEUE_THLD_CTRL 0x1c +#define QUEUE_THLD_CTRL_RESP_BUF_MASK GENMASK(15, 8) +#define QUEUE_THLD_CTRL_RESP_BUF(x) (((x) - 1) << 8) + +#define DATA_BUFFER_THLD_CTRL 0x20 +#define DATA_BUFFER_THLD_CTRL_RX_BUF GENMASK(11, 8) + +#define IBI_QUEUE_CTRL 0x24 +#define IBI_MR_REQ_REJECT 0x2C +#define IBI_SIR_REQ_REJECT 0x30 +#define IBI_REQ_REJECT_ALL GENMASK(31, 0) + +#define RESET_CTRL 0x34 +#define RESET_CTRL_IBI_QUEUE BIT(5) +#define RESET_CTRL_RX_FIFO BIT(4) +#define RESET_CTRL_TX_FIFO BIT(3) +#define RESET_CTRL_RESP_QUEUE BIT(2) +#define RESET_CTRL_CMD_QUEUE BIT(1) +#define RESET_CTRL_SOFT BIT(0) + +#define SLV_EVENT_CTRL 0x38 +#define INTR_STATUS 0x3c +#define INTR_STATUS_EN 0x40 +#define INTR_SIGNAL_EN 0x44 +#define INTR_FORCE 0x48 +#define INTR_BUSOWNER_UPDATE_STAT BIT(13) +#define INTR_IBI_UPDATED_STAT BIT(12) +#define INTR_READ_REQ_RECV_STAT BIT(11) +#define INTR_DEFSLV_STAT BIT(10) +#define INTR_TRANSFER_ERR_STAT BIT(9) +#define INTR_DYN_ADDR_ASSGN_STAT BIT(8) +#define INTR_CCC_UPDATED_STAT BIT(6) +#define INTR_TRANSFER_ABORT_STAT BIT(5) +#define INTR_RESP_READY_STAT BIT(4) +#define INTR_CMD_QUEUE_READY_STAT BIT(3) +#define INTR_IBI_THLD_STAT BIT(2) +#define INTR_RX_THLD_STAT BIT(1) +#define INTR_TX_THLD_STAT BIT(0) +#define INTR_ALL (INTR_BUSOWNER_UPDATE_STAT | \ + INTR_IBI_UPDATED_STAT | \ + INTR_READ_REQ_RECV_STAT | \ + INTR_DEFSLV_STAT | \ + INTR_TRANSFER_ERR_STAT | \ + INTR_DYN_ADDR_ASSGN_STAT | \ + INTR_CCC_UPDATED_STAT | \ + INTR_TRANSFER_ABORT_STAT | \ + INTR_RESP_READY_STAT | \ + INTR_CMD_QUEUE_READY_STAT | \ + INTR_IBI_THLD_STAT | \ + INTR_TX_THLD_STAT | \ + INTR_RX_THLD_STAT) + +#define INTR_MASTER_MASK (INTR_TRANSFER_ERR_STAT | \ + INTR_RESP_READY_STAT) + +#define QUEUE_STATUS_LEVEL 0x4c +#define QUEUE_STATUS_IBI_STATUS_CNT(x) (((x) & GENMASK(28, 24)) >> 24) +#define QUEUE_STATUS_IBI_BUF_BLR(x) (((x) & GENMASK(23, 16)) >> 16) +#define QUEUE_STATUS_LEVEL_RESP(x) (((x) & GENMASK(15, 8)) >> 8) +#define QUEUE_STATUS_LEVEL_CMD(x) ((x) & GENMASK(7, 0)) + +#define DATA_BUFFER_STATUS_LEVEL 0x50 +#define DATA_BUFFER_STATUS_LEVEL_TX(x) ((x) & GENMASK(7, 0)) + +#define PRESENT_STATE 0x54 +#define CCC_DEVICE_STATUS 0x58 +#define DEVICE_ADDR_TABLE_POINTER 0x5c +#define DEVICE_ADDR_TABLE_DEPTH(x) (((x) & GENMASK(31, 16)) >> 16) +#define DEVICE_ADDR_TABLE_ADDR(x) ((x) & GENMASK(7, 0)) + +#define DEV_CHAR_TABLE_POINTER 0x60 +#define VENDOR_SPECIFIC_REG_POINTER 0x6c +#define SLV_PID_VALUE 0x74 +#define SLV_CHAR_CTRL 0x78 +#define SLV_MAX_LEN 0x7c +#define MAX_READ_TURNAROUND 0x80 +#define MAX_DATA_SPEED 0x84 +#define SLV_DEBUG_STATUS 0x88 +#define SLV_INTR_REQ 0x8c +#define DEVICE_CTRL_EXTENDED 0xb0 +#define SCL_I3C_OD_TIMING 0xb4 +#define SCL_I3C_PP_TIMING 0xb8 +#define SCL_I3C_TIMING_HCNT(x) (((x) << 16) & GENMASK(23, 16)) +#define SCL_I3C_TIMING_LCNT(x) ((x) & GENMASK(7, 0)) +#define SCL_I3C_TIMING_CNT_MIN 5 + +#define SCL_I2C_FM_TIMING 0xbc +#define SCL_I2C_FM_TIMING_HCNT(x) (((x) << 16) & GENMASK(31, 16)) +#define SCL_I2C_FM_TIMING_LCNT(x) ((x) & GENMASK(15, 0)) + +#define SCL_I2C_FMP_TIMING 0xc0 +#define SCL_I2C_FMP_TIMING_HCNT(x) (((x) << 16) & GENMASK(23, 16)) +#define SCL_I2C_FMP_TIMING_LCNT(x) ((x) & GENMASK(15, 0)) + +#define SCL_EXT_LCNT_TIMING 0xc8 +#define SCL_EXT_LCNT_4(x) (((x) << 24) & GENMASK(31, 24)) +#define SCL_EXT_LCNT_3(x) (((x) << 16) & GENMASK(23, 16)) +#define SCL_EXT_LCNT_2(x) (((x) << 8) & GENMASK(15, 8)) +#define SCL_EXT_LCNT_1(x) ((x) & GENMASK(7, 0)) + +#define SCL_EXT_TERMN_LCNT_TIMING 0xcc +#define BUS_FREE_TIMING 0xd4 +#define BUS_I3C_MST_FREE(x) ((x) & GENMASK(15, 0)) + +#define BUS_IDLE_TIMING 0xd8 +#define I3C_VER_ID 0xe0 +#define I3C_VER_TYPE 0xe4 +#define EXTENDED_CAPABILITY 0xe8 +#define SLAVE_CONFIG 0xec + +#define DEV_ADDR_TABLE_LEGACY_I2C_DEV BIT(31) +#define DEV_ADDR_TABLE_DYNAMIC_ADDR(x) (((x) << 16) & GENMASK(23, 16)) +#define DEV_ADDR_TABLE_STATIC_ADDR(x) ((x) & GENMASK(6, 0)) +#define DEV_ADDR_TABLE_LOC(start, idx) ((start) + ((idx) << 2)) + +#define MAX_DEVS 32 + +struct dw_i3c_master_caps { + u8 cmdfifodepth; + u8 datafifodepth; +}; + +struct dw_i3c_master { + struct i3c_master_controller base; + u16 maxdevs; + u16 datstartaddr; + u32 free_pos; + struct { + struct list_head list; + struct dw_i3c_xfer *cur; + spinlock_t lock; + } xferqueue; + struct dw_i3c_master_caps caps; + void __iomem *regs; + struct reset_control *core_rst; + struct clk *core_clk; + char version[5]; + char type[5]; + u8 addrs[MAX_DEVS]; +}; + +struct dw_i3c_i2c_dev_data { + u8 index; +}; + +irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id); +int dw_i3c_master_probe(struct dw_i3c_master *master, struct device *dev); +int dw_i3c_master_remove(struct dw_i3c_master *master); diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c index b532e2c..d66410d 100644 --- a/drivers/i3c/master/dw-i3c-master.c +++ b/drivers/i3c/master/dw-i3c-master.c @@ -12,184 +12,12 @@ #include <linux/errno.h> #include <linux/i3c/master.h> #include <linux/interrupt.h> -#include <linux/ioport.h> #include <linux/iopoll.h> #include <linux/list.h> #include <linux/module.h> -#include <linux/of.h> -#include <linux/platform_device.h> -#include <linux/reset.h> #include <linux/slab.h> -#define DEVICE_CTRL 0x0 -#define DEV_CTRL_ENABLE BIT(31) -#define DEV_CTRL_RESUME BIT(30) -#define DEV_CTRL_HOT_JOIN_NACK BIT(8) -#define DEV_CTRL_I2C_SLAVE_PRESENT BIT(7) - -#define DEVICE_ADDR 0x4 -#define DEV_ADDR_DYNAMIC_ADDR_VALID BIT(31) -#define DEV_ADDR_DYNAMIC(x) (((x) << 16) & GENMASK(22, 16)) - -#define HW_CAPABILITY 0x8 -#define COMMAND_QUEUE_PORT 0xc -#define COMMAND_PORT_TOC BIT(30) -#define COMMAND_PORT_READ_TRANSFER BIT(28) -#define COMMAND_PORT_SDAP BIT(27) -#define COMMAND_PORT_ROC BIT(26) -#define COMMAND_PORT_SPEED(x) (((x) << 21) & GENMASK(23, 21)) -#define COMMAND_PORT_DEV_INDEX(x) (((x) << 16) & GENMASK(20, 16)) -#define COMMAND_PORT_CP BIT(15) -#define COMMAND_PORT_CMD(x) (((x) << 7) & GENMASK(14, 7)) -#define COMMAND_PORT_TID(x) (((x) << 3) & GENMASK(6, 3)) - -#define COMMAND_PORT_ARG_DATA_LEN(x) (((x) << 16) & GENMASK(31, 16)) -#define COMMAND_PORT_ARG_DATA_LEN_MAX 65536 -#define COMMAND_PORT_TRANSFER_ARG 0x01 - -#define COMMAND_PORT_SDA_DATA_BYTE_3(x) (((x) << 24) & GENMASK(31, 24)) -#define COMMAND_PORT_SDA_DATA_BYTE_2(x) (((x) << 16) & GENMASK(23, 16)) -#define COMMAND_PORT_SDA_DATA_BYTE_1(x) (((x) << 8) & GENMASK(15, 8)) -#define COMMAND_PORT_SDA_BYTE_STRB_3 BIT(5) -#define COMMAND_PORT_SDA_BYTE_STRB_2 BIT(4) -#define COMMAND_PORT_SDA_BYTE_STRB_1 BIT(3) -#define COMMAND_PORT_SHORT_DATA_ARG 0x02 - -#define COMMAND_PORT_DEV_COUNT(x) (((x) << 21) & GENMASK(25, 21)) -#define COMMAND_PORT_ADDR_ASSGN_CMD 0x03 - -#define RESPONSE_QUEUE_PORT 0x10 -#define RESPONSE_PORT_ERR_STATUS(x) (((x) & GENMASK(31, 28)) >> 28) -#define RESPONSE_NO_ERROR 0 -#define RESPONSE_ERROR_CRC 1 -#define RESPONSE_ERROR_PARITY 2 -#define RESPONSE_ERROR_FRAME 3 -#define RESPONSE_ERROR_IBA_NACK 4 -#define RESPONSE_ERROR_ADDRESS_NACK 5 -#define RESPONSE_ERROR_OVER_UNDER_FLOW 6 -#define RESPONSE_ERROR_TRANSF_ABORT 8 -#define RESPONSE_ERROR_I2C_W_NACK_ERR 9 -#define RESPONSE_PORT_TID(x) (((x) & GENMASK(27, 24)) >> 24) -#define RESPONSE_PORT_DATA_LEN(x) ((x) & GENMASK(15, 0)) - -#define RX_TX_DATA_PORT 0x14 -#define IBI_QUEUE_STATUS 0x18 -#define QUEUE_THLD_CTRL 0x1c -#define QUEUE_THLD_CTRL_RESP_BUF_MASK GENMASK(15, 8) -#define QUEUE_THLD_CTRL_RESP_BUF(x) (((x) - 1) << 8) - -#define DATA_BUFFER_THLD_CTRL 0x20 -#define DATA_BUFFER_THLD_CTRL_RX_BUF GENMASK(11, 8) - -#define IBI_QUEUE_CTRL 0x24 -#define IBI_MR_REQ_REJECT 0x2C -#define IBI_SIR_REQ_REJECT 0x30 -#define IBI_REQ_REJECT_ALL GENMASK(31, 0) - -#define RESET_CTRL 0x34 -#define RESET_CTRL_IBI_QUEUE BIT(5) -#define RESET_CTRL_RX_FIFO BIT(4) -#define RESET_CTRL_TX_FIFO BIT(3) -#define RESET_CTRL_RESP_QUEUE BIT(2) -#define RESET_CTRL_CMD_QUEUE BIT(1) -#define RESET_CTRL_SOFT BIT(0) - -#define SLV_EVENT_CTRL 0x38 -#define INTR_STATUS 0x3c -#define INTR_STATUS_EN 0x40 -#define INTR_SIGNAL_EN 0x44 -#define INTR_FORCE 0x48 -#define INTR_BUSOWNER_UPDATE_STAT BIT(13) -#define INTR_IBI_UPDATED_STAT BIT(12) -#define INTR_READ_REQ_RECV_STAT BIT(11) -#define INTR_DEFSLV_STAT BIT(10) -#define INTR_TRANSFER_ERR_STAT BIT(9) -#define INTR_DYN_ADDR_ASSGN_STAT BIT(8) -#define INTR_CCC_UPDATED_STAT BIT(6) -#define INTR_TRANSFER_ABORT_STAT BIT(5) -#define INTR_RESP_READY_STAT BIT(4) -#define INTR_CMD_QUEUE_READY_STAT BIT(3) -#define INTR_IBI_THLD_STAT BIT(2) -#define INTR_RX_THLD_STAT BIT(1) -#define INTR_TX_THLD_STAT BIT(0) -#define INTR_ALL (INTR_BUSOWNER_UPDATE_STAT | \ - INTR_IBI_UPDATED_STAT | \ - INTR_READ_REQ_RECV_STAT | \ - INTR_DEFSLV_STAT | \ - INTR_TRANSFER_ERR_STAT | \ - INTR_DYN_ADDR_ASSGN_STAT | \ - INTR_CCC_UPDATED_STAT | \ - INTR_TRANSFER_ABORT_STAT | \ - INTR_RESP_READY_STAT | \ - INTR_CMD_QUEUE_READY_STAT | \ - INTR_IBI_THLD_STAT | \ - INTR_TX_THLD_STAT | \ - INTR_RX_THLD_STAT) - -#define INTR_MASTER_MASK (INTR_TRANSFER_ERR_STAT | \ - INTR_RESP_READY_STAT) - -#define QUEUE_STATUS_LEVEL 0x4c -#define QUEUE_STATUS_IBI_STATUS_CNT(x) (((x) & GENMASK(28, 24)) >> 24) -#define QUEUE_STATUS_IBI_BUF_BLR(x) (((x) & GENMASK(23, 16)) >> 16) -#define QUEUE_STATUS_LEVEL_RESP(x) (((x) & GENMASK(15, 8)) >> 8) -#define QUEUE_STATUS_LEVEL_CMD(x) ((x) & GENMASK(7, 0)) - -#define DATA_BUFFER_STATUS_LEVEL 0x50 -#define DATA_BUFFER_STATUS_LEVEL_TX(x) ((x) & GENMASK(7, 0)) - -#define PRESENT_STATE 0x54 -#define CCC_DEVICE_STATUS 0x58 -#define DEVICE_ADDR_TABLE_POINTER 0x5c -#define DEVICE_ADDR_TABLE_DEPTH(x) (((x) & GENMASK(31, 16)) >> 16) -#define DEVICE_ADDR_TABLE_ADDR(x) ((x) & GENMASK(7, 0)) - -#define DEV_CHAR_TABLE_POINTER 0x60 -#define VENDOR_SPECIFIC_REG_POINTER 0x6c -#define SLV_PID_VALUE 0x74 -#define SLV_CHAR_CTRL 0x78 -#define SLV_MAX_LEN 0x7c -#define MAX_READ_TURNAROUND 0x80 -#define MAX_DATA_SPEED 0x84 -#define SLV_DEBUG_STATUS 0x88 -#define SLV_INTR_REQ 0x8c -#define DEVICE_CTRL_EXTENDED 0xb0 -#define SCL_I3C_OD_TIMING 0xb4 -#define SCL_I3C_PP_TIMING 0xb8 -#define SCL_I3C_TIMING_HCNT(x) (((x) << 16) & GENMASK(23, 16)) -#define SCL_I3C_TIMING_LCNT(x) ((x) & GENMASK(7, 0)) -#define SCL_I3C_TIMING_CNT_MIN 5 - -#define SCL_I2C_FM_TIMING 0xbc -#define SCL_I2C_FM_TIMING_HCNT(x) (((x) << 16) & GENMASK(31, 16)) -#define SCL_I2C_FM_TIMING_LCNT(x) ((x) & GENMASK(15, 0)) - -#define SCL_I2C_FMP_TIMING 0xc0 -#define SCL_I2C_FMP_TIMING_HCNT(x) (((x) << 16) & GENMASK(23, 16)) -#define SCL_I2C_FMP_TIMING_LCNT(x) ((x) & GENMASK(15, 0)) - -#define SCL_EXT_LCNT_TIMING 0xc8 -#define SCL_EXT_LCNT_4(x) (((x) << 24) & GENMASK(31, 24)) -#define SCL_EXT_LCNT_3(x) (((x) << 16) & GENMASK(23, 16)) -#define SCL_EXT_LCNT_2(x) (((x) << 8) & GENMASK(15, 8)) -#define SCL_EXT_LCNT_1(x) ((x) & GENMASK(7, 0)) - -#define SCL_EXT_TERMN_LCNT_TIMING 0xcc -#define BUS_FREE_TIMING 0xd4 -#define BUS_I3C_MST_FREE(x) ((x) & GENMASK(15, 0)) - -#define BUS_IDLE_TIMING 0xd8 -#define I3C_VER_ID 0xe0 -#define I3C_VER_TYPE 0xe4 -#define EXTENDED_CAPABILITY 0xe8 -#define SLAVE_CONFIG 0xec - -#define DEV_ADDR_TABLE_LEGACY_I2C_DEV BIT(31) -#define DEV_ADDR_TABLE_DYNAMIC_ADDR(x) (((x) << 16) & GENMASK(23, 16)) -#define DEV_ADDR_TABLE_STATIC_ADDR(x) ((x) & GENMASK(6, 0)) -#define DEV_ADDR_TABLE_LOC(start, idx) ((start) + ((idx) << 2)) - -#define MAX_DEVS 32 +#include "dw-i3c-core.h" #define I3C_BUS_SDR1_SCL_RATE 8000000 #define I3C_BUS_SDR2_SCL_RATE 6000000 @@ -201,11 +29,6 @@ #define XFER_TIMEOUT (msecs_to_jiffies(1000)) -struct dw_i3c_master_caps { - u8 cmdfifodepth; - u8 datafifodepth; -}; - struct dw_i3c_cmd { u32 cmd_lo; u32 cmd_hi; @@ -224,29 +47,6 @@ struct dw_i3c_xfer { struct dw_i3c_cmd cmds[0]; }; -struct dw_i3c_master { - struct i3c_master_controller base; - u16 maxdevs; - u16 datstartaddr; - u32 free_pos; - struct { - struct list_head list; - struct dw_i3c_xfer *cur; - spinlock_t lock; - } xferqueue; - struct dw_i3c_master_caps caps; - void __iomem *regs; - struct reset_control *core_rst; - struct clk *core_clk; - char version[5]; - char type[5]; - u8 addrs[MAX_DEVS]; -}; - -struct dw_i3c_i2c_dev_data { - u8 index; -}; - static u8 even_parity(u8 p) { p ^= p >> 4; @@ -1071,7 +871,7 @@ static u32 dw_i3c_master_i2c_funcs(struct i3c_master_controller *m) return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; } -static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id) +irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id) { struct dw_i3c_master *master = dev_id; u32 status; @@ -1091,6 +891,7 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; } +EXPORT_SYMBOL_GPL(dw_i3c_master_irq_handler); static const struct i3c_master_controller_ops dw_mipi_i3c_ops = { .bus_init = dw_i3c_master_bus_init, @@ -1108,48 +909,9 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = { .i2c_funcs = dw_i3c_master_i2c_funcs, }; -static int dw_i3c_probe(struct platform_device *pdev) +int dw_i3c_master_probe(struct dw_i3c_master *master, struct device *dev) { - struct dw_i3c_master *master; - struct resource *res; - int ret, irq; - - master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL); - if (!master) - return -ENOMEM; - - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - master->regs = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(master->regs)) - return PTR_ERR(master->regs); - - master->core_clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(master->core_clk)) - return PTR_ERR(master->core_clk); - - master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev, - "core_rst"); - if (IS_ERR(master->core_rst)) - return PTR_ERR(master->core_rst); - - ret = clk_prepare_enable(master->core_clk); - if (ret) - goto err_disable_core_clk; - - reset_control_deassert(master->core_rst); - - spin_lock_init(&master->xferqueue.lock); - INIT_LIST_HEAD(&master->xferqueue.list); - - writel(INTR_ALL, master->regs + INTR_STATUS); - irq = platform_get_irq(pdev, 0); - ret = devm_request_irq(&pdev->dev, irq, - dw_i3c_master_irq_handler, 0, - dev_name(&pdev->dev), master); - if (ret) - goto err_assert_rst; - - platform_set_drvdata(pdev, master); + int ret; /* Information regarding the FIFOs/QUEUEs depth */ ret = readl(master->regs + QUEUE_STATUS_LEVEL); @@ -1163,54 +925,17 @@ static int dw_i3c_probe(struct platform_device *pdev) master->maxdevs = ret >> 16; master->free_pos = GENMASK(master->maxdevs - 1, 0); - ret = i3c_master_register(&master->base, &pdev->dev, - &dw_mipi_i3c_ops, false); - if (ret) - goto err_assert_rst; - - return 0; - -err_assert_rst: - reset_control_assert(master->core_rst); - -err_disable_core_clk: - clk_disable_unprepare(master->core_clk); - - return ret; + return i3c_master_register(&master->base, dev, + &dw_mipi_i3c_ops, false); } +EXPORT_SYMBOL_GPL(dw_i3c_master_probe); -static int dw_i3c_remove(struct platform_device *pdev) +int dw_i3c_master_remove(struct dw_i3c_master *master) { - struct dw_i3c_master *master = platform_get_drvdata(pdev); - int ret; - - ret = i3c_master_unregister(&master->base); - if (ret) - return ret; - - reset_control_assert(master->core_rst); - - clk_disable_unprepare(master->core_clk); - - return 0; + return i3c_master_unregister(&master->base); } - -static const struct of_device_id dw_i3c_master_of_match[] = { - { .compatible = "snps,dw-i3c-master-1.00a", }, - {}, -}; -MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match); - -static struct platform_driver dw_i3c_driver = { - .probe = dw_i3c_probe, - .remove = dw_i3c_remove, - .driver = { - .name = "dw-i3c-master", - .of_match_table = of_match_ptr(dw_i3c_master_of_match), - }, -}; -module_platform_driver(dw_i3c_driver); +EXPORT_SYMBOL_GPL(dw_i3c_master_remove); MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>"); -MODULE_DESCRIPTION("DesignWare MIPI I3C driver"); +MODULE_DESCRIPTION("DesignWare MIPI I3C Master"); MODULE_LICENSE("GPL v2"); diff --git a/drivers/i3c/master/dw-i3c-platdrv.c b/drivers/i3c/master/dw-i3c-platdrv.c new file mode 100644 index 0000000..f4fb5a8 --- /dev/null +++ b/drivers/i3c/master/dw-i3c-platdrv.c @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018 Synopsys, Inc. and/or its affiliates. + * + * Author: Vitor Soares <vitor.soares@synopsys.com> + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/i3c/master.h> +#include <linux/interrupt.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/slab.h> + +#include "dw-i3c-core.h" + +static int dw_i3c_probe(struct platform_device *pdev) +{ + struct dw_i3c_master *master; + struct resource *res; + int ret, irq; + + master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL); + if (!master) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + master->regs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(master->regs)) + return PTR_ERR(master->regs); + + master->core_clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(master->core_clk)) + return PTR_ERR(master->core_clk); + + master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev, + "core_rst"); + if (IS_ERR(master->core_rst)) + return PTR_ERR(master->core_rst); + + ret = clk_prepare_enable(master->core_clk); + if (ret) + goto err_disable_core_clk; + + reset_control_deassert(master->core_rst); + + spin_lock_init(&master->xferqueue.lock); + INIT_LIST_HEAD(&master->xferqueue.list); + + irq = platform_get_irq(pdev, 0); + ret = devm_request_irq(&pdev->dev, irq, + dw_i3c_master_irq_handler, 0, + dev_name(&pdev->dev), master); + if (ret) + goto err_assert_rst; + + platform_set_drvdata(pdev, master); + + ret = dw_i3c_master_probe(master, &pdev->dev); + if (ret) + goto err_assert_rst; + + return 0; + +err_assert_rst: + reset_control_assert(master->core_rst); + +err_disable_core_clk: + clk_disable_unprepare(master->core_clk); + + return ret; +} + +static int dw_i3c_remove(struct platform_device *pdev) +{ + struct dw_i3c_master *master = platform_get_drvdata(pdev); + int ret; + + ret = dw_i3c_master_remove(master); + if (ret) + return ret; + + reset_control_assert(master->core_rst); + + clk_disable_unprepare(master->core_clk); + + return 0; +} + +static const struct of_device_id dw_i3c_master_of_match[] = { + { .compatible = "snps,dw-i3c-master-1.00a", }, + {}, +}; +MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match); + +static struct platform_driver dw_i3c_driver = { + .probe = dw_i3c_probe, + .remove = dw_i3c_remove, + .driver = { + .name = "dw-i3c-master", + .of_match_table = of_match_ptr(dw_i3c_master_of_match), + }, +}; +module_platform_driver(dw_i3c_driver); + +MODULE_AUTHOR("Vitor Soares <vitor.soares@synopsys.com>"); +MODULE_DESCRIPTION("DesignWare MIPI I3C Bus driver"); +MODULE_LICENSE("GPL v2");