From: Serge Semin <[email protected]>
There are three DW I2C controllers embedded into the Baikal-T1 SoC. Two
of them are normal with standard DW I2C IP-core configurations and registers
accessible over normal MMIO space - so they are acceptable by the available
DW I2C driver with no modification. But there is a third, which is a bit
different. Its registers are indirectly accessed be means of "command/data
in/data out" registers tuple. In order to have it also supported by the DW
I2C driver, we must modify the code a bit. This is a main purpose of this
patchset.
First of all traditionally we replaced the legacy plain text-based dt-binding
file with yaml-based one. Then we found and fixed a bug in the DW I2C FIFO size
detection algorithm which tried to do it too early before dw_readl/dw_writel
methods could be used. Finally we introduced a platform-specific flag
ACCESS_INDIRECT, which would enable the indirect access to the DW I2C registers
implemented for one of the Baikal-T1 SoC DW I2C controllers. See the commit
message of the corresponding patch for details.
This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
commit 98d54f81e36b ("Linux 5.6-rc4").
Signed-off-by: Serge Semin <[email protected]>
Signed-off-by: Alexey Malahov <[email protected]>
Cc: Maxim Kaurkin <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Ramil Zaripov <[email protected]>
Cc: Ekaterina Skachko <[email protected]>
Cc: Vadim Vlasov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Jarkko Nikula <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Serge Semin (6):
scripts/dtc: check: Add additional i2c reg flags support
dt-bindings: i2c: Replace DW I2C legacy bindings with YAML-based one
dt-bindings: i2c: dw: Add Baikal-T1 SoC I2C controller
i2c: designware: Detect the FIFO size in the common code
i2c: designware: Discard i2c_dw_read_comp_param() function
i2c: designware: Add Baikal-T1 SoC I2C controller support
.../bindings/i2c/i2c-designware.txt | 73 --------
.../bindings/i2c/snps,designware-i2c.yaml | 158 ++++++++++++++++++
drivers/i2c/busses/i2c-designware-common.c | 107 ++++++++++--
drivers/i2c/busses/i2c-designware-core.h | 16 +-
drivers/i2c/busses/i2c-designware-master.c | 3 +
drivers/i2c/busses/i2c-designware-platdrv.c | 25 +--
drivers/i2c/busses/i2c-designware-slave.c | 3 +
scripts/dtc/checks.c | 13 +-
8 files changed, 280 insertions(+), 118 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware.txt
create mode 100644 Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
--
2.25.1
First of all, I got only 3 out of 6 patches. Are you sure you properly prepared
the series?
On Fri, Mar 06, 2020 at 04:19:49PM +0300, [email protected] wrote:
> From: Serge Semin <[email protected]>
Same comment as per DMA series, try next time to link the cover letter to the
series correctly.
> There are three DW I2C controllers embedded into the Baikal-T1 SoC. Two
> of them are normal with standard DW I2C IP-core configurations and registers
> accessible over normal MMIO space - so they are acceptable by the available
> DW I2C driver with no modification.
> But there is a third, which is a bit
> different. Its registers are indirectly accessed be means of "command/data
> in/data out" registers tuple. In order to have it also supported by the DW
> I2C driver, we must modify the code a bit. This is a main purpose of this
> patchset.
>
> First of all traditionally we replaced the legacy plain text-based dt-binding
> file with yaml-based one. Then we found and fixed a bug in the DW I2C FIFO size
> detection algorithm which tried to do it too early before dw_readl/dw_writel
> methods could be used.
So far so good (looks like, I think colleagues of mine and myself will review
individual patches later on).
> Finally we introduced a platform-specific flag
> ACCESS_INDIRECT, which would enable the indirect access to the DW I2C registers
> implemented for one of the Baikal-T1 SoC DW I2C controllers. See the commit
> message of the corresponding patch for details.
This is quite questionable. In Intel SoCs we have indirect I?C controllers to
access (inside PMIC, for example). The approach used to do that is usually to
have an IPC mechanism and specific bus controller driver. See i2c-cht-wc.c for
instance.
I'm not sure if it makes a lot of duplication and if actually switching I?C
DesignWare driver to regmap API will solve it. At least that is the second
approach I would consider.
But I'll wait others to comment on this. We have to settle the solution before
going further.
> This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> commit 98d54f81e36b ("Linux 5.6-rc4").
`git format-patch --base ...` should do the job.
> Signed-off-by: Serge Semin <[email protected]>
> Signed-off-by: Alexey Malahov <[email protected]>
Same comment as per UART patch. Who is the Alexey in relation to the work done?
--
With Best Regards,
Andy Shevchenko
Hello Andy,
Finally I've thought this through reasonably conformed with the changes
requested in the framework of the other patchsets. My comments are
below.
On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:
> First of all, I got only 3 out of 6 patches. Are you sure you properly prepared
> the series?
>
> On Fri, Mar 06, 2020 at 04:19:49PM +0300, [email protected] wrote:
> > From: Serge Semin <[email protected]>
>
> Same comment as per DMA series, try next time to link the cover letter to the
> series correctly.
>
> > There are three DW I2C controllers embedded into the Baikal-T1 SoC. Two
> > of them are normal with standard DW I2C IP-core configurations and registers
> > accessible over normal MMIO space - so they are acceptable by the available
> > DW I2C driver with no modification.
>
> > But there is a third, which is a bit
> > different. Its registers are indirectly accessed be means of "command/data
> > in/data out" registers tuple. In order to have it also supported by the DW
> > I2C driver, we must modify the code a bit. This is a main purpose of this
> > patchset.
> >
> > First of all traditionally we replaced the legacy plain text-based dt-binding
> > file with yaml-based one. Then we found and fixed a bug in the DW I2C FIFO size
> > detection algorithm which tried to do it too early before dw_readl/dw_writel
> > methods could be used.
>
> So far so good (looks like, I think colleagues of mine and myself will review
> individual patches later on).
>
> > Finally we introduced a platform-specific flag
> > ACCESS_INDIRECT, which would enable the indirect access to the DW I2C registers
> > implemented for one of the Baikal-T1 SoC DW I2C controllers. See the commit
> > message of the corresponding patch for details.
>
> This is quite questionable. In Intel SoCs we have indirect I?C controllers to
> access (inside PMIC, for example). The approach used to do that is usually to
> have an IPC mechanism and specific bus controller driver. See i2c-cht-wc.c for
> instance.
>
> I'm not sure if it makes a lot of duplication and if actually switching I?C
> DesignWare driver to regmap API will solve it. At least that is the second
> approach I would consider.
>
> But I'll wait others to comment on this. We have to settle the solution before
> going further.
>
As I see the others have not comments.) Anyway I see your point and having the
regmap-based interface might be better than the approach I've suggested
in this patchset particularly seeing that our DW i2c IP registers are
hidden behind a system controller register space.
In order to follow your proposition to create a dedicated regmap and to supply
it to the DW i2c driver, I have to redevelop not only this patchset, but
also an adjacent drivers. In particular the changes will concern the
MFD-based System Controller driver (which will instantiate this DW i2c
controller device), Clocks Control Unit drivers set, and a few
others. The whole alteration I described in the RFC:
https://lkml.org/lkml/2020/3/22/393
You've been in Cc there, so fill free to send your comments regarding
the changes I suggested. Though this time I hope the solution will
satisfy everyone, who had issues with patchsets I've recently sent.
Getting back to your comment in the framework of this patchset. The approach
used for CHT Whiskey Cove i2c isn't fully suitable in our case for
the reason of the DW I2C controller nature. DW I2C controller is a generic
controller and used on many different platforms, while AFAICS CHT Whiskey Cove
I2C is the SoC-specific used to access a charger-IC. So in the former case we
may have an arbitrary set of i2c-slaves connected to the controller on
different platforms, while on the latter one - there is a fixed set of
slaves. In addition due to the same reason the DW I2C IP might be
embedded into different sub-blocks on different platforms, while the CHT
Whiskey Cove I2C is known to be a part of Intel CHT WC SoC PMIC.
For instance Baikal-T1 SoC has one DW I2C controller embedded into the
System Controller with indirectly accessible registers and two DW I2C
interfaces with normal memory mapped registers. Due to this in case of DW I2C
driver we can't just "suck" the regmap out from a parental MFD or
anywhere else as it's done in the CHT Whiskey Cove I2C driver, but instead
we should somehow supply a regmap pointer to the driver.
Taking into account all of these we can utilize a combined approach
implemented in ./drivers/i2c/busses/i2c-cht-wc.c and
drivers/mfd/intel_quark_i2c_gpio.c . I'll add a regmap pointer field to the
"struct dw_i2c_platform_data" structure, so in case if there is no
IORESOURCE_MEM resources available (platform_get_resource() fails), we
try to get a regmap pointer from the platform data. If there is no valid
regmap available, then completely fail the driver probe procedure. Though
due to this alteration I'll have to change the
dw_i2c_platform_data.i2c_scl_freq field usage a bit. In case if it's
zero, then call i2c_parse_fw_timings(). This won't hurt ACPI or dt-less
platforms, but will let us cover a case when regmap is set while i2c
clock frequency is supposed to be taken from the kernel firmware (like
dtb, etc).
So if you are Ok with this, I'll send a v2 patchset with corresponding
alteration implemented.
Regards,
-Sergey
> > This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> > commit 98d54f81e36b ("Linux 5.6-rc4").
>
> `git format-patch --base ...` should do the job.
>
> > Signed-off-by: Serge Semin <[email protected]>
> > Signed-off-by: Alexey Malahov <[email protected]>
>
> Same comment as per UART patch. Who is the Alexey in relation to the work done?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, Mar 31, 2020 at 02:48:24PM +0300, Sergey Semin wrote:
> Hello Andy,
>
> Finally I've thought this through reasonably conformed with the changes
> requested in the framework of the other patchsets. My comments are
> below.
>
> On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:
> > First of all, I got only 3 out of 6 patches. Are you sure you properly prepared
> > the series?
> >
> > On Fri, Mar 06, 2020 at 04:19:49PM +0300, [email protected] wrote:
> > > From: Serge Semin <[email protected]>
> >
> > Same comment as per DMA series, try next time to link the cover letter to the
> > series correctly.
> >
> > > There are three DW I2C controllers embedded into the Baikal-T1 SoC. Two
> > > of them are normal with standard DW I2C IP-core configurations and registers
> > > accessible over normal MMIO space - so they are acceptable by the available
> > > DW I2C driver with no modification.
> >
> > > But there is a third, which is a bit
> > > different. Its registers are indirectly accessed be means of "command/data
> > > in/data out" registers tuple. In order to have it also supported by the DW
> > > I2C driver, we must modify the code a bit. This is a main purpose of this
> > > patchset.
> > >
> > > First of all traditionally we replaced the legacy plain text-based dt-binding
> > > file with yaml-based one. Then we found and fixed a bug in the DW I2C FIFO size
> > > detection algorithm which tried to do it too early before dw_readl/dw_writel
> > > methods could be used.
> >
> > So far so good (looks like, I think colleagues of mine and myself will review
> > individual patches later on).
> >
> > > Finally we introduced a platform-specific flag
> > > ACCESS_INDIRECT, which would enable the indirect access to the DW I2C registers
> > > implemented for one of the Baikal-T1 SoC DW I2C controllers. See the commit
> > > message of the corresponding patch for details.
> >
> > This is quite questionable. In Intel SoCs we have indirect I?C controllers to
> > access (inside PMIC, for example). The approach used to do that is usually to
> > have an IPC mechanism and specific bus controller driver. See i2c-cht-wc.c for
> > instance.
> >
> > I'm not sure if it makes a lot of duplication and if actually switching I?C
> > DesignWare driver to regmap API will solve it. At least that is the second
> > approach I would consider.
> >
> > But I'll wait others to comment on this. We have to settle the solution before
> > going further.
> >
>
> As I see the others have not comments.) Anyway I see your point and having the
> regmap-based interface might be better than the approach I've suggested
> in this patchset particularly seeing that our DW i2c IP registers are
> hidden behind a system controller register space.
>
> In order to follow your proposition to create a dedicated regmap and to supply
> it to the DW i2c driver, I have to redevelop not only this patchset, but
> also an adjacent drivers. In particular the changes will concern the
> MFD-based System Controller driver (which will instantiate this DW i2c
> controller device), Clocks Control Unit drivers set, and a few
> others. The whole alteration I described in the RFC:
> https://lkml.org/lkml/2020/3/22/393
> You've been in Cc there, so fill free to send your comments regarding
> the changes I suggested. Though this time I hope the solution will
> satisfy everyone, who had issues with patchsets I've recently sent.
>
> Getting back to your comment in the framework of this patchset. The approach
> used for CHT Whiskey Cove i2c isn't fully suitable in our case for
> the reason of the DW I2C controller nature. DW I2C controller is a generic
> controller and used on many different platforms, while AFAICS CHT Whiskey Cove
> I2C is the SoC-specific used to access a charger-IC. So in the former case we
> may have an arbitrary set of i2c-slaves connected to the controller on
> different platforms, while on the latter one - there is a fixed set of
> slaves. In addition due to the same reason the DW I2C IP might be
> embedded into different sub-blocks on different platforms, while the CHT
> Whiskey Cove I2C is known to be a part of Intel CHT WC SoC PMIC.
> For instance Baikal-T1 SoC has one DW I2C controller embedded into the
> System Controller with indirectly accessible registers and two DW I2C
> interfaces with normal memory mapped registers. Due to this in case of DW I2C
> driver we can't just "suck" the regmap out from a parental MFD or
> anywhere else as it's done in the CHT Whiskey Cove I2C driver, but instead
> we should somehow supply a regmap pointer to the driver.
>
> Taking into account all of these we can utilize a combined approach
> implemented in ./drivers/i2c/busses/i2c-cht-wc.c and
> drivers/mfd/intel_quark_i2c_gpio.c . I'll add a regmap pointer field to the
> "struct dw_i2c_platform_data" structure, so in case if there is no
> IORESOURCE_MEM resources available (platform_get_resource() fails), we
> try to get a regmap pointer from the platform data. If there is no valid
> regmap available, then completely fail the driver probe procedure. Though
> due to this alteration I'll have to change the
> dw_i2c_platform_data.i2c_scl_freq field usage a bit. In case if it's
> zero, then call i2c_parse_fw_timings(). This won't hurt ACPI or dt-less
> platforms, but will let us cover a case when regmap is set while i2c
> clock frequency is supposed to be taken from the kernel firmware (like
> dtb, etc).
>
> So if you are Ok with this, I'll send a v2 patchset with corresponding
> alteration implemented.
I was thinking about something like this:
1/ core driver (library + master + slave) is converted to use regmap
2/ platform and PCI driver may provide regmap MMIO
3/ your glue driver will provide different regmap accessors
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 31, 2020 at 05:25:30PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2020 at 02:48:24PM +0300, Sergey Semin wrote:
> > Hello Andy,
> >
> > Finally I've thought this through reasonably conformed with the changes
> > requested in the framework of the other patchsets. My comments are
> > below.
> >
> > On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:
> > > First of all, I got only 3 out of 6 patches. Are you sure you properly prepared
> > > the series?
> > >
> > > On Fri, Mar 06, 2020 at 04:19:49PM +0300, [email protected] wrote:
> > > > From: Serge Semin <[email protected]>
> > >
> > > Same comment as per DMA series, try next time to link the cover letter to the
> > > series correctly.
> > >
> > > > There are three DW I2C controllers embedded into the Baikal-T1 SoC. Two
> > > > of them are normal with standard DW I2C IP-core configurations and registers
> > > > accessible over normal MMIO space - so they are acceptable by the available
> > > > DW I2C driver with no modification.
> > >
> > > > But there is a third, which is a bit
> > > > different. Its registers are indirectly accessed be means of "command/data
> > > > in/data out" registers tuple. In order to have it also supported by the DW
> > > > I2C driver, we must modify the code a bit. This is a main purpose of this
> > > > patchset.
> > > >
> > > > First of all traditionally we replaced the legacy plain text-based dt-binding
> > > > file with yaml-based one. Then we found and fixed a bug in the DW I2C FIFO size
> > > > detection algorithm which tried to do it too early before dw_readl/dw_writel
> > > > methods could be used.
> > >
> > > So far so good (looks like, I think colleagues of mine and myself will review
> > > individual patches later on).
> > >
> > > > Finally we introduced a platform-specific flag
> > > > ACCESS_INDIRECT, which would enable the indirect access to the DW I2C registers
> > > > implemented for one of the Baikal-T1 SoC DW I2C controllers. See the commit
> > > > message of the corresponding patch for details.
> > >
> > > This is quite questionable. In Intel SoCs we have indirect I?C controllers to
> > > access (inside PMIC, for example). The approach used to do that is usually to
> > > have an IPC mechanism and specific bus controller driver. See i2c-cht-wc.c for
> > > instance.
> > >
> > > I'm not sure if it makes a lot of duplication and if actually switching I?C
> > > DesignWare driver to regmap API will solve it. At least that is the second
> > > approach I would consider.
> > >
> > > But I'll wait others to comment on this. We have to settle the solution before
> > > going further.
> > >
> >
> > As I see the others have not comments.) Anyway I see your point and having the
> > regmap-based interface might be better than the approach I've suggested
> > in this patchset particularly seeing that our DW i2c IP registers are
> > hidden behind a system controller register space.
> >
> > In order to follow your proposition to create a dedicated regmap and to supply
> > it to the DW i2c driver, I have to redevelop not only this patchset, but
> > also an adjacent drivers. In particular the changes will concern the
> > MFD-based System Controller driver (which will instantiate this DW i2c
> > controller device), Clocks Control Unit drivers set, and a few
> > others. The whole alteration I described in the RFC:
> > https://lkml.org/lkml/2020/3/22/393
> > You've been in Cc there, so fill free to send your comments regarding
> > the changes I suggested. Though this time I hope the solution will
> > satisfy everyone, who had issues with patchsets I've recently sent.
> >
> > Getting back to your comment in the framework of this patchset. The approach
> > used for CHT Whiskey Cove i2c isn't fully suitable in our case for
> > the reason of the DW I2C controller nature. DW I2C controller is a generic
> > controller and used on many different platforms, while AFAICS CHT Whiskey Cove
> > I2C is the SoC-specific used to access a charger-IC. So in the former case we
> > may have an arbitrary set of i2c-slaves connected to the controller on
> > different platforms, while on the latter one - there is a fixed set of
> > slaves. In addition due to the same reason the DW I2C IP might be
> > embedded into different sub-blocks on different platforms, while the CHT
> > Whiskey Cove I2C is known to be a part of Intel CHT WC SoC PMIC.
> > For instance Baikal-T1 SoC has one DW I2C controller embedded into the
> > System Controller with indirectly accessible registers and two DW I2C
> > interfaces with normal memory mapped registers. Due to this in case of DW I2C
> > driver we can't just "suck" the regmap out from a parental MFD or
> > anywhere else as it's done in the CHT Whiskey Cove I2C driver, but instead
> > we should somehow supply a regmap pointer to the driver.
> >
> > Taking into account all of these we can utilize a combined approach
> > implemented in ./drivers/i2c/busses/i2c-cht-wc.c and
> > drivers/mfd/intel_quark_i2c_gpio.c . I'll add a regmap pointer field to the
> > "struct dw_i2c_platform_data" structure, so in case if there is no
> > IORESOURCE_MEM resources available (platform_get_resource() fails), we
> > try to get a regmap pointer from the platform data. If there is no valid
> > regmap available, then completely fail the driver probe procedure. Though
> > due to this alteration I'll have to change the
> > dw_i2c_platform_data.i2c_scl_freq field usage a bit. In case if it's
> > zero, then call i2c_parse_fw_timings(). This won't hurt ACPI or dt-less
> > platforms, but will let us cover a case when regmap is set while i2c
> > clock frequency is supposed to be taken from the kernel firmware (like
> > dtb, etc).
> >
> > So if you are Ok with this, I'll send a v2 patchset with corresponding
> > alteration implemented.
>
> I was thinking about something like this:
>
> 1/ core driver (library + master + slave) is converted to use regmap
Yes. I also intended to do this just by altering the dw_readl() and
dw_writel() methods to work over regmap IO methods if regmap is
available.
> 2/ platform and PCI driver may provide regmap MMIO
Regmap pointer will be also a part of "struct dw_i2c_dev". So if PCI
code intends the regmap-based access to the controller registers, then
it shall just initialize the regmap pointer in the private i2c-designware data
instance of the dw_i2c_dev structure. So, yes, this is also covered by
my solution. Though the PCI code will be left untouched, since I can't
predict a particular regmap-related use-case of it.
> 3/ your glue driver will provide different regmap accessors
I was thinking of developing a more generic version so any platform
with a specific access to the DW I2C register could use it just by supplying
the regmap pointer in the dw_i2c_platform_data structure. Our DW I2C
controller also perfectly fits to the generic i2c-designware-platdrv.c
driver, so implementing an additional glue-layer would be too much seeing
the difference only in the registers mapping.
Let me explain the difference of our solutions. In case of implementing
the glue layer, as you suggest, I would have to do it in a way like the DW PCIe
driver is designed. I would need to move the code of current dw_i2c_plat_probe()
function to a dedicated method named like dw_i2c_plat_init(pdev, !regmap!),
while former method dw_i2c_plat_probe() would just call
dw_i2c_plat_init(pdev, !NULL!). Then I would have to create a dedicated
glue-driver - i2c-designware-bt1drv.c, which would be bound to a
"baikal,t1-sys-i2c" device, try to find a Baikal-T1 System Controller
device node (though this would be just a parent device), then would get
it' syscon regmap handler, then would initialize a dedicated regmap handler to
indirectly access the DW I2C controller register, then it would call the
dw_i2c_plat_init(pdev, !regmap!) method with new regmap handler passed
(though the new regmap passing could be also implemented over the
platform_data pointer). Also seeing you already have a platform-specific
parts in the generic i2c-designware-platdrv.c driver (like ACPI-based
platforms and Microsemi Ocelot SoC), there might raise a necessity to
unpin that specifics to a dedicated method, since my glue-layer
wouldn't need that checks and initializations. Such alteration won't
be that easy to implement and regression errors prone, since I don't have
other platforms to test it.
In case of my solution the whole glue-layer part would be moved to
the MFD-based Baikal-T1 System Controller driver and a generic
platform_data-based interface would be implemented, which would just
need to alter the registers mapping part of the i2c-designware-platdrv.c
driver. Note that that part would need to be fixed in case of any solution.
So comparing these too approaches, I would select a one, which would
need less common code modifications and would provide a generic
solution. As I see it would be a platform_data-based design. What do you
think?
Regards,
-Sergey
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, Mar 31, 2020 at 7:29 PM Sergey Semin
<[email protected]> wrote:
> On Tue, Mar 31, 2020 at 05:25:30PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 31, 2020 at 02:48:24PM +0300, Sergey Semin wrote:
> > > On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:
...
> > > As I see the others have not comments.) Anyway I see your point and having the
> > > regmap-based interface might be better than the approach I've suggested
> > > in this patchset particularly seeing that our DW i2c IP registers are
> > > hidden behind a system controller register space.
> > >
> > > In order to follow your proposition to create a dedicated regmap and to supply
> > > it to the DW i2c driver, I have to redevelop not only this patchset, but
> > > also an adjacent drivers. In particular the changes will concern the
> > > MFD-based System Controller driver (which will instantiate this DW i2c
> > > controller device), Clocks Control Unit drivers set, and a few
> > > others. The whole alteration I described in the RFC:
> > > https://lkml.org/lkml/2020/3/22/393
> > > You've been in Cc there, so fill free to send your comments regarding
> > > the changes I suggested. Though this time I hope the solution will
> > > satisfy everyone, who had issues with patchsets I've recently sent.
> > >
> > > Getting back to your comment in the framework of this patchset. The approach
> > > used for CHT Whiskey Cove i2c isn't fully suitable in our case for
> > > the reason of the DW I2C controller nature. DW I2C controller is a generic
> > > controller and used on many different platforms, while AFAICS CHT Whiskey Cove
> > > I2C is the SoC-specific used to access a charger-IC. So in the former case we
> > > may have an arbitrary set of i2c-slaves connected to the controller on
> > > different platforms, while on the latter one - there is a fixed set of
> > > slaves. In addition due to the same reason the DW I2C IP might be
> > > embedded into different sub-blocks on different platforms, while the CHT
> > > Whiskey Cove I2C is known to be a part of Intel CHT WC SoC PMIC.
> > > For instance Baikal-T1 SoC has one DW I2C controller embedded into the
> > > System Controller with indirectly accessible registers and two DW I2C
> > > interfaces with normal memory mapped registers. Due to this in case of DW I2C
> > > driver we can't just "suck" the regmap out from a parental MFD or
> > > anywhere else as it's done in the CHT Whiskey Cove I2C driver, but instead
> > > we should somehow supply a regmap pointer to the driver.
> > >
> > > Taking into account all of these we can utilize a combined approach
> > > implemented in ./drivers/i2c/busses/i2c-cht-wc.c and
> > > drivers/mfd/intel_quark_i2c_gpio.c . I'll add a regmap pointer field to the
> > > "struct dw_i2c_platform_data" structure, so in case if there is no
> > > IORESOURCE_MEM resources available (platform_get_resource() fails), we
> > > try to get a regmap pointer from the platform data. If there is no valid
> > > regmap available, then completely fail the driver probe procedure. Though
> > > due to this alteration I'll have to change the
> > > dw_i2c_platform_data.i2c_scl_freq field usage a bit. In case if it's
> > > zero, then call i2c_parse_fw_timings(). This won't hurt ACPI or dt-less
> > > platforms, but will let us cover a case when regmap is set while i2c
> > > clock frequency is supposed to be taken from the kernel firmware (like
> > > dtb, etc).
Whiskey Cove case provides an I²C controller with specific access. That's it.
In your case you will need similar glue driver which will utilize
DesignWare core driver.
> > > So if you are Ok with this, I'll send a v2 patchset with corresponding
> > > alteration implemented.
> >
> > I was thinking about something like this:
> >
>
> > 1/ core driver (library + master + slave) is converted to use regmap
>
> Yes. I also intended to do this just by altering the dw_readl() and
> dw_writel() methods to work over regmap IO methods if regmap is
> available.
Why? Simple always use regmap API calls. Moreover, regmap provides
update() which may give a leverage in some cases by dropping r-m-w
code.
Core part take a pointer to regmap when instantiate (probe) a driver
on the device.
> > 2/ platform and PCI driver may provide regmap MMIO
>
> Regmap pointer will be also a part of "struct dw_i2c_dev".
Yes.
> So if PCI
> code intends the regmap-based access to the controller registers, then
> it shall just initialize the regmap pointer in the private i2c-designware data
> instance of the dw_i2c_dev structure.
Why data is being involved here?
> So, yes, this is also covered by
> my solution. Though the PCI code will be left untouched, since I can't
> predict a particular regmap-related use-case of it.
> > 3/ your glue driver will provide different regmap accessors
>
> I was thinking of developing a more generic version so any platform
> with a specific access to the DW I2C register could use it just by supplying
> the regmap pointer in the dw_i2c_platform_data structure.
regmap is not a platform data.
> Our DW I2C
> controller also perfectly fits to the generic i2c-designware-platdrv.c
> driver, so implementing an additional glue-layer would be too much seeing
> the difference only in the registers mapping.
Register accessing, mapping as far as I understood is the same.
However, with regmap layer it just an implementation detail of certain
glue driver.
> Let me explain the difference of our solutions. In case of implementing
> the glue layer, as you suggest, I would have to do it in a way like the DW PCIe
> driver is designed. I would need to move the code of current dw_i2c_plat_probe()
> function to a dedicated method named like dw_i2c_plat_init(pdev, !regmap!),
> while former method dw_i2c_plat_probe() would just call
> dw_i2c_plat_init(pdev, !NULL!). Then I would have to create a dedicated
> glue-driver - i2c-designware-bt1drv.c, which would be bound to a
> "baikal,t1-sys-i2c" device, try to find a Baikal-T1 System Controller
> device node (though this would be just a parent device), then would get
> it' syscon regmap handler, then would initialize a dedicated regmap handler to
> indirectly access the DW I2C controller register, then it would call the
> dw_i2c_plat_init(pdev, !regmap!) method with new regmap handler passed
> (though the new regmap passing could be also implemented over the
> platform_data pointer). Also seeing you already have a platform-specific
> parts in the generic i2c-designware-platdrv.c driver (like ACPI-based
> platforms and Microsemi Ocelot SoC), there might raise a necessity to
> unpin that specifics to a dedicated method, since my glue-layer
> wouldn't need that checks and initializations. Such alteration won't
> be that easy to implement and regression errors prone, since I don't have
> other platforms to test it.
>
> In case of my solution the whole glue-layer part would be moved to
> the MFD-based Baikal-T1 System Controller driver and a generic
> platform_data-based interface would be implemented, which would just
> need to alter the registers mapping part of the i2c-designware-platdrv.c
> driver. Note that that part would need to be fixed in case of any solution.
>
> So comparing these too approaches, I would select a one, which would
> need less common code modifications and would provide a generic
> solution. As I see it would be a platform_data-based design. What do you
> think?
I think you may look at the existing examples how drivers are
utilizing regmap layer.
Along with that look how glue (or let's say quirk in your case)
drivers are implemented, for example USB or SDHCI comes to my mind.
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 31, 2020 at 08:17:28PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 31, 2020 at 7:29 PM Sergey Semin
> <[email protected]> wrote:
> > On Tue, Mar 31, 2020 at 05:25:30PM +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 31, 2020 at 02:48:24PM +0300, Sergey Semin wrote:
> > > > On Fri, Mar 06, 2020 at 03:54:45PM +0200, Andy Shevchenko wrote:
>
> ...
>
> > > > As I see the others have not comments.) Anyway I see your point and having the
> > > > regmap-based interface might be better than the approach I've suggested
> > > > in this patchset particularly seeing that our DW i2c IP registers are
> > > > hidden behind a system controller register space.
> > > >
> > > > In order to follow your proposition to create a dedicated regmap and to supply
> > > > it to the DW i2c driver, I have to redevelop not only this patchset, but
> > > > also an adjacent drivers. In particular the changes will concern the
> > > > MFD-based System Controller driver (which will instantiate this DW i2c
> > > > controller device), Clocks Control Unit drivers set, and a few
> > > > others. The whole alteration I described in the RFC:
> > > > https://lkml.org/lkml/2020/3/22/393
> > > > You've been in Cc there, so fill free to send your comments regarding
> > > > the changes I suggested. Though this time I hope the solution will
> > > > satisfy everyone, who had issues with patchsets I've recently sent.
> > > >
> > > > Getting back to your comment in the framework of this patchset. The approach
> > > > used for CHT Whiskey Cove i2c isn't fully suitable in our case for
> > > > the reason of the DW I2C controller nature. DW I2C controller is a generic
> > > > controller and used on many different platforms, while AFAICS CHT Whiskey Cove
> > > > I2C is the SoC-specific used to access a charger-IC. So in the former case we
> > > > may have an arbitrary set of i2c-slaves connected to the controller on
> > > > different platforms, while on the latter one - there is a fixed set of
> > > > slaves. In addition due to the same reason the DW I2C IP might be
> > > > embedded into different sub-blocks on different platforms, while the CHT
> > > > Whiskey Cove I2C is known to be a part of Intel CHT WC SoC PMIC.
> > > > For instance Baikal-T1 SoC has one DW I2C controller embedded into the
> > > > System Controller with indirectly accessible registers and two DW I2C
> > > > interfaces with normal memory mapped registers. Due to this in case of DW I2C
> > > > driver we can't just "suck" the regmap out from a parental MFD or
> > > > anywhere else as it's done in the CHT Whiskey Cove I2C driver, but instead
> > > > we should somehow supply a regmap pointer to the driver.
> > > >
> > > > Taking into account all of these we can utilize a combined approach
> > > > implemented in ./drivers/i2c/busses/i2c-cht-wc.c and
> > > > drivers/mfd/intel_quark_i2c_gpio.c . I'll add a regmap pointer field to the
> > > > "struct dw_i2c_platform_data" structure, so in case if there is no
> > > > IORESOURCE_MEM resources available (platform_get_resource() fails), we
> > > > try to get a regmap pointer from the platform data. If there is no valid
> > > > regmap available, then completely fail the driver probe procedure. Though
> > > > due to this alteration I'll have to change the
> > > > dw_i2c_platform_data.i2c_scl_freq field usage a bit. In case if it's
> > > > zero, then call i2c_parse_fw_timings(). This won't hurt ACPI or dt-less
> > > > platforms, but will let us cover a case when regmap is set while i2c
> > > > clock frequency is supposed to be taken from the kernel firmware (like
> > > > dtb, etc).
>
> Whiskey Cove case provides an I?C controller with specific access. That's it.
> In your case you will need similar glue driver which will utilize
> DesignWare core driver.
>
> > > > So if you are Ok with this, I'll send a v2 patchset with corresponding
> > > > alteration implemented.
> > >
> > > I was thinking about something like this:
> > >
> >
> > > 1/ core driver (library + master + slave) is converted to use regmap
> >
> > Yes. I also intended to do this just by altering the dw_readl() and
> > dw_writel() methods to work over regmap IO methods if regmap is
> > available.
>
> Why? Simple always use regmap API calls. Moreover, regmap provides
> update() which may give a leverage in some cases by dropping r-m-w
> code.
>
See the last comment.
> Core part take a pointer to regmap when instantiate (probe) a driver
> on the device.
>
> > > 2/ platform and PCI driver may provide regmap MMIO
> >
> > Regmap pointer will be also a part of "struct dw_i2c_dev".
>
> Yes.
>
> > So if PCI
> > code intends the regmap-based access to the controller registers, then
> > it shall just initialize the regmap pointer in the private i2c-designware data
> > instance of the dw_i2c_dev structure.
>
> Why data is being involved here?
>
This data is the driver private data. Have a better look at what I suggested.
In my solution regmap is just an optional way of accessing the registers.
The main part is left unchanged: IO-accessors usage to read/write MMIO registers
range in currently implemented dw_readl()/dw_writel().
> > So, yes, this is also covered by
> > my solution. Though the PCI code will be left untouched, since I can't
> > predict a particular regmap-related use-case of it.
>
> > > 3/ your glue driver will provide different regmap accessors
> >
> > I was thinking of developing a more generic version so any platform
> > with a specific access to the DW I2C register could use it just by supplying
> > the regmap pointer in the dw_i2c_platform_data structure.
>
> regmap is not a platform data.
Nothing prevents us from adding a regmap pointer to the platform_data and according
to the already defined platform_data's for other devices it could. Anyway see
my last comment.
>
> > Our DW I2C
> > controller also perfectly fits to the generic i2c-designware-platdrv.c
> > driver, so implementing an additional glue-layer would be too much seeing
> > the difference only in the registers mapping.
>
> Register accessing, mapping as far as I understood is the same.
> However, with regmap layer it just an implementation detail of certain
> glue driver.
>
Yes, registers region structure is the same while the implementation is
different. Though taking that implementation specifics into account is different
in my and yours solutions. Anyway see my last comment.
> > Let me explain the difference of our solutions. In case of implementing
> > the glue layer, as you suggest, I would have to do it in a way like the DW PCIe
> > driver is designed. I would need to move the code of current dw_i2c_plat_probe()
> > function to a dedicated method named like dw_i2c_plat_init(pdev, !regmap!),
> > while former method dw_i2c_plat_probe() would just call
> > dw_i2c_plat_init(pdev, !NULL!). Then I would have to create a dedicated
> > glue-driver - i2c-designware-bt1drv.c, which would be bound to a
> > "baikal,t1-sys-i2c" device, try to find a Baikal-T1 System Controller
> > device node (though this would be just a parent device), then would get
> > it' syscon regmap handler, then would initialize a dedicated regmap handler to
> > indirectly access the DW I2C controller register, then it would call the
> > dw_i2c_plat_init(pdev, !regmap!) method with new regmap handler passed
> > (though the new regmap passing could be also implemented over the
> > platform_data pointer). Also seeing you already have a platform-specific
> > parts in the generic i2c-designware-platdrv.c driver (like ACPI-based
> > platforms and Microsemi Ocelot SoC), there might raise a necessity to
> > unpin that specifics to a dedicated method, since my glue-layer
> > wouldn't need that checks and initializations. Such alteration won't
> > be that easy to implement and regression errors prone, since I don't have
> > other platforms to test it.
> >
> > In case of my solution the whole glue-layer part would be moved to
> > the MFD-based Baikal-T1 System Controller driver and a generic
> > platform_data-based interface would be implemented, which would just
> > need to alter the registers mapping part of the i2c-designware-platdrv.c
> > driver. Note that that part would need to be fixed in case of any solution.
> >
> > So comparing these too approaches, I would select a one, which would
> > need less common code modifications and would provide a generic
> > solution. As I see it would be a platform_data-based design. What do you
> > think?
>
> I think you may look at the existing examples how drivers are
> utilizing regmap layer.
> Along with that look how glue (or let's say quirk in your case)
> drivers are implemented, for example USB or SDHCI comes to my mind.
>
I know how drivers use regmaps. I also know the glue layers design.
If you took a closer look at my suggestion you would find out that first
I referred to a code which utilize the glue layers, second I was describing
a solution which would have caused less changes, requires less work,
wouldn't have needed so many alterations of the common code, which
potentially would have caused less problems.
Regarding your design of the solution. Basically you want me to convert
the DW I2C driver to using the regmap API. It shall involve the IO-accessors
usage conversion so instead of calling the dw_readl() and dw_writel(),
regmap_{read,write}() are supposed to be utilized. You also want me to alter
the DW I2C generic platform and PCI code so it would convert the MMIO regions
to simple regmap_init_mmio'es. Finally you want me to implement our DW I2C
specifics in a dedicated "glue"-layer, and if not doing as I suggested
of separating the dw_i2c_plat_probe() functionality (which is glue layer
design), but to develop it as a quirk exporting a custom regmap pointer.
As I already said your solution needs much work, so will take greater
time to implement and debug. It's much more complicated than mine, and
potentially may cause more errors. While conceptually your suggestion
seems a bit more suitable, my solution is also acceptable and perfectly
fine to be part of the kernel. So I'll see what I can do in implementing
your suggestion. If I have enough time to work on that, I'll do it.
Otherwise I'll be working on my solution. Either of them will be sent in v2.
Regards,
-Sergey
> --
> With Best Regards,
> Andy Shevchenko
Initially this has been a small patchset which embedded the Baikal-T1
System I2C support into the DW APB I2C driver as is by using a simplest
way. After a short discussion with Andy we decided to implement what he
suggested (introduce regmap-based accessors and create a glue driver) and
even more than that to provide some cleanups of the code. So here is what
this patchset consists of.
First of all we've found out that current implementation of scripts/dtc
didn't support i2c dt nodes with 10bit and slave flags set in the
reg property. You'll see an error if you try to dt_binding_check it.
So the very first patch fixes the problem by adding these flags support
into the check_i2c_bus_reg() method.
Traditionally we converted the plain text-based DT binding to the DT schema
and added Baikal-T1 System I2C device support there. This required to mark
the reg property redundant for Baikal-T1 I2C since its reg-space is
indirectly accessed by means of the System Controller cmd/read/write
registers.
Then as Andy suggested we replaced the Synopsys DW APB I2C common driver
registers IO accessors into the regmap API methods. This doesn't change
the code logic much, though in two places we managed to replace some bulky
peaces of code with a ready-to-use regmap methods.
Additionally before adding the glue layer API we initiated a set of cleanups:
- Define components of the multi-object drivers (like i2c-designware-core.o
and i2c-designware-paltform.o) with using `-y` suffixed makefile
variables instead of `-objs` suffixed one. This is encouraged by
Documentation/kbuild/makefiles.rst text since `-objs` is supposed to be used
to build host programs.
- Make DW I2C slave driver depended on the DW I2C core code instead of the
platform one, which it really is.
- Move Intel Baytrail semaphore feature to the platform if-clause of the
kernel config.
After this we finally can introduce the glue layer API for the DW APB I2C
platform driver. So there are three methods exported from the driver:
i2c_dw_plat_setup(), i2c_dw_plat_clear(), &i2c_dw_plat_dev_pm_ops to
setup, cleanup and add PM operations to the glue driven I2C device. Before
setting the platform DW I2C device up the glue probe code is supposed to
create an instance of DW I2C device generic object and pre-initialize
its `struct device` pointer together with optional platform-specific
flags. In addition to that we converted the MSCC Ocelot SoC I2C specific
code into the glue layer seeing it's really too specific and, which is more
important, isn't that complicated so we could unpin it without much of
worrying to break something.
Meanwhile we discovered that MODEL_CHERRYTRAIL and MODEL_MASK actually
were no longer used in the code. MODEL_MSCC flag has been discarded since
the MSCC Ocelot I2C code conversion to the glue driver. So now we can get
rid of all the MODEL-specific flags.
Finally we introduced a glue driver with Baikal-T1 System I2C device
support. The driver probe tries to find a syscon regmap, creates the DW
APB I2C regmap based on it and passes it further to the DW I2C device
descriptor. Then it does normal DW APB I2C platform setup by calling a
generic setup method. Cleanup is straightforward. It's just calling a
generic DW APB I2C clean method.
This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
tag: v5.7-rc4
Note new vendor prefix for Baikal-T1 System I2C device will be added in
the framework of the next patchset:
https://lkml.org/lkml/2020/5/6/1047
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Maxim Kaurkin <[email protected]>
Cc: Pavel Parkhomenko <[email protected]>
Cc: Ramil Zaripov <[email protected]>
Cc: Ekaterina Skachko <[email protected]>
Cc: Vadim Vlasov <[email protected]>
Cc: Alexey Kolotnikov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Changelog v2:
- Fix the SoB tags.
- Use a shorter summary describing the bindings convertion patch.
- Patch "i2c: designware: Detect the FIFO size in the common code" has
been acked by Jarkko and applied by Wolfram to for-next so drop it from
the set.
- Patch "i2c: designware: Discard i2c_dw_read_comp_param() function" has
been acked by Jarkko and applied by Wolfram to for-next so drop it from
the set.
- Make sure that "mscc,ocelot-i2c" compatible node may have up to two
registers space defined in the DT node, while normal DW I2C controller
will have only one registers space.
- Add "mscc,ocelot-i2c" DT schema example to test the previous fix.
- Declare "unevaluatedProperties" property instead of
"additionalProperties" one in the DT schema.
- Due to the previous fix we can now discard the dummy boolean properties
declaration, since the proper type evaluation will be performed by the
generic i2c-controller.yaml schema.
- Refactor the DW I2C APB driver related series to address the Andies
notes.
- Convert DW APB I2C driver to using regmap instead of handwritten
accessors.
- Use `-y` to build multi-object DW APB drivers.
- Fix DW APB I2C slave code dependency. It should depend on
I2C_DESIGNWARE_CORE instead I2C_DESIGNWARE_PLATFORM.
- Move Baytrail semaphore config to the platform if-clause.
- Introduce a glue-layer platform driver API.
- Unpin Microsemi Ocelot I2C code into a glue driver.
- Remove MODEL_CHERRYTRAIL and MODEL_MASK as no longer needed.
- Add support for custom regmap passed from glue driver.
- Add Baikal-T1 System I2C support in a dedicated glue layer driver.
Serge Semin (12):
scripts/dtc: check: Add 10bit/slave i2c reg flags support
dt-bindings: i2c: Convert DW I2C binding to DT schema
dt-bindings: i2c: dw: Add Baikal-T1 SoC I2C controller
i2c: designware: Convert driver to using regmap API
i2c: designware: Use `-y` to build multi-object modules
i2c: designware: slave: Set DW I2C core module dependency
i2c: designware: Move Baytrail sem config to the platform if-clause
i2c: designware: Introduce platform drivers glue layer interface
i2c: designware: Unpin Microsemi Ocelot I2C into a glue driver
i2c: designware: Discard Cherry Trail model flag
i2c: designware: Use provided regmap instead of reg resource
i2c: designware: Add Baikal-T1 System I2C glue driver
.../bindings/i2c/i2c-designware.txt | 73 -------
.../bindings/i2c/snps,designware-i2c.yaml | 164 ++++++++++++++++
drivers/i2c/busses/Kconfig | 56 ++++--
drivers/i2c/busses/Makefile | 19 +-
drivers/i2c/busses/i2c-designware-bt1.c | 129 +++++++++++++
drivers/i2c/busses/i2c-designware-common.c | 178 +++++++++++++-----
drivers/i2c/busses/i2c-designware-core.h | 26 ++-
drivers/i2c/busses/i2c-designware-master.c | 125 ++++++------
drivers/i2c/busses/i2c-designware-mscc.c | 83 ++++++++
drivers/i2c/busses/i2c-designware-pcidrv.c | 1 -
drivers/i2c/busses/i2c-designware-platdrv.c | 132 ++++++-------
drivers/i2c/busses/i2c-designware-platdrv.h | 16 ++
drivers/i2c/busses/i2c-designware-slave.c | 77 ++++----
scripts/dtc/checks.c | 13 +-
14 files changed, 759 insertions(+), 333 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware.txt
create mode 100644 Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
create mode 100644 drivers/i2c/busses/i2c-designware-bt1.c
create mode 100644 drivers/i2c/busses/i2c-designware-mscc.c
create mode 100644 drivers/i2c/busses/i2c-designware-platdrv.h
--
2.25.1
Add the "baikal,bt1-sys-i2c" compatible string to the DW I2C binding and
make sure the reg property isn't required in this case because the
controller is embedded into the Baikal-T1 System Controller. The rest of
the DW APB I2C properties are compatible and can be freely used to describe
the Baikal-T1 I2C controller dts-node.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
---
Rob, I had to remove your acked-by tag because of the changes introduced
in v2 of the patch.
Changelog v2:
- Make the reg property being optional if it's Baikal-T1 System I2C DT
node.
---
.../devicetree/bindings/i2c/snps,designware-i2c.yaml | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
index 8d4e5fccbd1c..579964098eb9 100644
--- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
@@ -21,6 +21,15 @@ allOf:
properties:
reg:
maxItems: 1
+ - if:
+ properties:
+ compatible:
+ not:
+ contains:
+ const: baikal,bt1-sys-i2c
+ then:
+ required:
+ - reg
properties:
compatible:
@@ -31,6 +40,8 @@ properties:
items:
- const: mscc,ocelot-i2c
- const: snps,designware-i2c
+ - description: Baikal-T1 SoC System I2C controller
+ const: baikal,bt1-sys-i2c
reg:
minItems: 1
@@ -98,7 +109,6 @@ unevaluatedProperties: false
required:
- compatible
- - reg
- "#address-cells"
- "#size-cells"
- interrupts
--
2.25.1
Modern device tree bindings are supposed to be created as YAML-files
in accordance with dt-schema. This commit replaces Synopsys DW I2C
legacy bare text bindings with YAML file. As before the bindings file
states that the corresponding dts node is supposed to be compatible
either with generic DW I2C controller or with Microsemi Ocelot SoC I2C
one, to have registers, interrupts and clocks properties. In addition
the node may have clock-frequency, i2c-sda-hold-time-ns,
i2c-scl-falling-time-ns and i2c-sda-falling-time-ns optional properties.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
---
Changelog v2:
- Make sure that "mscc,ocelot-i2c" compatible node may have up to two
registers space defined, while normal DW I2C controller will have only
one registers space.
- Add "mscc,ocelot-i2c" example to test the previous fix.
- Declare "unevaluatedProperties" property instead of
"additionalProperties" one.
- Due to the previous fix we can now discard the dummy boolean properties
definitions, since the proper type evaluation will be performed by the
generic i2c-controller.yaml schema.
---
.../bindings/i2c/i2c-designware.txt | 73 ---------
.../bindings/i2c/snps,designware-i2c.yaml | 154 ++++++++++++++++++
2 files changed, 154 insertions(+), 73 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware.txt
create mode 100644 Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
deleted file mode 100644
index 08be4d3846e5..000000000000
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ /dev/null
@@ -1,73 +0,0 @@
-* Synopsys DesignWare I2C
-
-Required properties :
-
- - compatible : should be "snps,designware-i2c"
- or "mscc,ocelot-i2c" with "snps,designware-i2c" for fallback
- - reg : Offset and length of the register set for the device
- - interrupts : <IRQ> where IRQ is the interrupt number.
- - clocks : phandles for the clocks, see the description of clock-names below.
- The phandle for the "ic_clk" clock is required. The phandle for the "pclk"
- clock is optional. If a single clock is specified but no clock-name, it is
- the "ic_clk" clock. If both clocks are listed, the "ic_clk" must be first.
-
-Recommended properties :
-
- - clock-frequency : desired I2C bus clock frequency in Hz.
-
-Optional properties :
-
- - clock-names : Contains the names of the clocks:
- "ic_clk", for the core clock used to generate the external I2C clock.
- "pclk", the interface clock, required for register access.
-
- - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold
- time, named ICPU_CFG:TWI_DELAY in the datasheet.
-
- - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
- This option is only supported in hardware blocks version 1.11a or newer and
- on Microsemi SoCs ("mscc,ocelot-i2c" compatible).
-
- - i2c-scl-falling-time-ns : should contain the SCL falling time in nanoseconds.
- This value which is by default 300ns is used to compute the tLOW period.
-
- - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
- This value which is by default 300ns is used to compute the tHIGH period.
-
-Examples :
-
- i2c@f0000 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "snps,designware-i2c";
- reg = <0xf0000 0x1000>;
- interrupts = <11>;
- clock-frequency = <400000>;
- };
-
- i2c@1120000 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "snps,designware-i2c";
- reg = <0x1120000 0x1000>;
- interrupt-parent = <&ictl>;
- interrupts = <12 1>;
- clock-frequency = <400000>;
- i2c-sda-hold-time-ns = <300>;
- i2c-sda-falling-time-ns = <300>;
- i2c-scl-falling-time-ns = <300>;
- };
-
- i2c@1120000 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0x2000 0x100>;
- clock-frequency = <400000>;
- clocks = <&i2cclk>;
- interrupts = <0>;
-
- eeprom@64 {
- compatible = "linux,slave-24c02";
- reg = <0x40000064>;
- };
- };
diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
new file mode 100644
index 000000000000..8d4e5fccbd1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/snps,designware-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare APB I2C Controller
+
+maintainers:
+ - Jarkko Nikula <[email protected]>
+
+allOf:
+ - $ref: /schemas/i2c/i2c-controller.yaml#
+ - if:
+ properties:
+ compatible:
+ not:
+ contains:
+ const: mscc,ocelot-i2c
+ then:
+ properties:
+ reg:
+ maxItems: 1
+
+properties:
+ compatible:
+ oneOf:
+ - description: Generic Synopsys DesignWare I2C controller
+ const: snps,designware-i2c
+ - description: Microsemi Ocelot SoCs I2C controller
+ items:
+ - const: mscc,ocelot-i2c
+ - const: snps,designware-i2c
+
+ reg:
+ minItems: 1
+ items:
+ - description: DW APB I2C controller memory mapped registers
+ - description: |
+ ICPU_CFG:TWI_DELAY registers to setup the SDA hold time.
+ This registers are specific to the Ocelot I2C-controller.
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ minItems: 1
+ items:
+ - description: I2C controller reference clock source
+ - description: APB interface clock source
+
+ clock-names:
+ minItems: 1
+ items:
+ - const: ref
+ - const: pclk
+
+ resets:
+ maxItems: 1
+
+ clock-frequency:
+ description: Desired I2C bus clock frequency in Hz
+ enum: [100000, 400000, 1000000, 3400000]
+ default: 400000
+
+ i2c-sda-hold-time-ns:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ The property should contain the SDA hold time in nanoseconds. This option
+ is only supported in hardware blocks version 1.11a or newer or on
+ Microsemi SoCs.
+
+ i2c-scl-falling-time-ns:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ The property should contain the SCL falling time in nanoseconds.
+ This value is used to compute the tLOW period.
+ default: 300
+
+ i2c-sda-falling-time-ns:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: |
+ The property should contain the SDA falling time in nanoseconds.
+ This value is used to compute the tHIGH period.
+ default: 300
+
+ dmas:
+ items:
+ - description: TX DMA Channel
+ - description: RX DMA Channel
+
+ dma-names:
+ items:
+ - const: tx
+ - const: rx
+
+unevaluatedProperties: false
+
+required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+ - interrupts
+
+examples:
+ - |
+ i2c@f0000 {
+ compatible = "snps,designware-i2c";
+ reg = <0xf0000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = <11>;
+ clock-frequency = <400000>;
+ };
+ - |
+ i2c@1120000 {
+ compatible = "snps,designware-i2c";
+ reg = <0x1120000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = <12 1>;
+ clock-frequency = <400000>;
+ i2c-sda-hold-time-ns = <300>;
+ i2c-sda-falling-time-ns = <300>;
+ i2c-scl-falling-time-ns = <300>;
+ };
+ - |
+ i2c@2000 {
+ compatible = "snps,designware-i2c";
+ reg = <0x2000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clock-frequency = <400000>;
+ clocks = <&i2cclk>;
+ interrupts = <0>;
+
+ eeprom@64 {
+ compatible = "linux,slave-24c02";
+ reg = <0x40000064>;
+ };
+ };
+ - |
+ i2c@100400 {
+ compatible = "mscc,ocelot-i2c", "snps,designware-i2c";
+ reg = <0x100400 0x100>, <0x198 0x8>;
+ pinctrl-0 = <&i2c_pins>;
+ pinctrl-names = "default";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = <8>;
+ clocks = <&ahb_clk>;
+ };
+...
--
2.25.1
Since commit 4f8272802739 ("Documentation: update kbuild loadable modules
goals & examples") `-objs` is fitted for building host programs, lets
change DW I2C core, platform and PCI driver kbuild directives to using
`-y`, which more straightforward for device drivers. By doing so we can
discard the ifeq construction in favor to the more natural and less bulky
`<module>-$(CONFIG_X) += x.o`
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/Makefile | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 25d60889713c..c6813d7b2780 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -48,16 +48,15 @@ obj-$(CONFIG_I2C_CADENCE) += i2c-cadence.o
obj-$(CONFIG_I2C_CBUS_GPIO) += i2c-cbus-gpio.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
obj-$(CONFIG_I2C_DAVINCI) += i2c-davinci.o
-obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
-i2c-designware-core-objs := i2c-designware-common.o i2c-designware-master.o
-ifeq ($(CONFIG_I2C_DESIGNWARE_SLAVE),y)
-i2c-designware-core-objs += i2c-designware-slave.o
-endif
-obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
-i2c-designware-platform-objs := i2c-designware-platdrv.o
+obj-$(CONFIG_I2C_DESIGNWARE_CORE) += i2c-designware-core.o
+i2c-designware-core-y := i2c-designware-common.o
+i2c-designware-core-y += i2c-designware-master.o
+i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-slave.o
+obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
+i2c-designware-platform-y := i2c-designware-platdrv.o
i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
-obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o
-i2c-designware-pci-objs := i2c-designware-pcidrv.o
+obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o
+i2c-designware-pci-y := i2c-designware-pcidrv.o
obj-$(CONFIG_I2C_DIGICOLOR) += i2c-digicolor.o
obj-$(CONFIG_I2C_EFM32) += i2c-efm32.o
obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o
--
2.25.1
Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
platform driver. It's a bit confusing to see it's config in the menu at
some separated place with no reference to the platform code. Lets move the
config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
the config menu will display the feature right below the DW I2C platform
driver item and will indent it to the right so signifying its belonging.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 368aa64e9266..ed6927c4c540 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
config I2C_DESIGNWARE_PLATFORM
tristate "Synopsys DesignWare Platform"
- select I2C_DESIGNWARE_CORE
depends on (ACPI && COMMON_CLK) || !ACPI
+ select I2C_DESIGNWARE_CORE
help
If you say yes to this option, support will be included for the
Synopsys DesignWare I2C adapter.
@@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
This driver can also be built as a module. If so, the module
will be called i2c-designware-platform.
+if I2C_DESIGNWARE_PLATFORM
+
+config I2C_DESIGNWARE_BAYTRAIL
+ bool "Intel Baytrail I2C semaphore support"
+ depends on ACPI
+ depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
+ (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
+ help
+ This driver enables managed host access to the PMIC I2C bus on select
+ Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
+ the host to request uninterrupted access to the PMIC's I2C bus from
+ the platform firmware controlling it. You should say Y if running on
+ a BayTrail system using the AXP288.
+
+endif # I2C_DESIGNWARE_PLATFORM
+
config I2C_DESIGNWARE_SLAVE
bool "Synopsys DesignWare Slave"
depends on I2C_DESIGNWARE_CORE
@@ -561,18 +577,6 @@ config I2C_DESIGNWARE_PCI
This driver can also be built as a module. If so, the module
will be called i2c-designware-pci.
-config I2C_DESIGNWARE_BAYTRAIL
- bool "Intel Baytrail I2C semaphore support"
- depends on ACPI
- depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
- (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
- help
- This driver enables managed host access to the PMIC I2C bus on select
- Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
- the host to request uninterrupted access to the PMIC's I2C bus from
- the platform firmware controlling it. You should say Y if running on
- a BayTrail system using the AXP288.
-
config I2C_DIGICOLOR
tristate "Conexant Digicolor I2C driver"
depends on ARCH_DIGICOLOR || COMPILE_TEST
--
2.25.1
Seeing the DW I2C driver is using flags-based accessors with two
conditional clauses it would be better to replace them with the regmap
API IO methods and to initialize the regmap object with read/write
callbacks specific to the controller registers map implementation. This
will be also handy for the drivers with non-standard registers mapping
(like an embedded into the Baikal-T1 System Controller DW I2C block, which
glue-driver is a part of this series).
As before the driver tries to detect the mapping setup at probe stage and
creates a regmap object accordingly, which will be used by the rest of the
code to correctly access the controller registers. In two places it was
appropriate to convert the hand-written read-modify-write and
read-poll-loop design patterns to the corresponding regmap API
ready-to-use methods.
Note the regmap IO methods return value is checked only at the probe
stage. The rest of the code won't do this because basically we have
MMIO-based regmap so non of the read/write methods can fail (this also
won't be needed for the Baikal-T1-specific I2C controller).
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/Kconfig | 1 +
drivers/i2c/busses/i2c-designware-common.c | 171 +++++++++++++++------
drivers/i2c/busses/i2c-designware-core.h | 18 +--
drivers/i2c/busses/i2c-designware-master.c | 125 ++++++++-------
drivers/i2c/busses/i2c-designware-slave.c | 77 +++++-----
5 files changed, 239 insertions(+), 153 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2ddca08f8a76..14368c46cb63 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -526,6 +526,7 @@ config I2C_DAVINCI
config I2C_DESIGNWARE_CORE
tristate
+ select REGMAP
config I2C_DESIGNWARE_PLATFORM
tristate "Synopsys DesignWare Platform"
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index c70c6fc09ee3..35c5ad7e274e 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -15,6 +15,7 @@
#include <linux/err.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/regmap.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pm_runtime.h>
@@ -53,44 +54,82 @@ static char *abort_sources[] = {
"incorrect slave-transmitter mode configuration",
};
-u32 dw_readl(struct dw_i2c_dev *dev, int offset)
+static int dw_reg_read(void *context, unsigned int reg, unsigned int *val)
{
- u32 value;
+ struct dw_i2c_dev *dev = context;
- if (dev->flags & ACCESS_16BIT)
- value = readw_relaxed(dev->base + offset) |
- (readw_relaxed(dev->base + offset + 2) << 16);
- else
- value = readl_relaxed(dev->base + offset);
+ *val = readl_relaxed(dev->base + reg);
- if (dev->flags & ACCESS_SWAP)
- return swab32(value);
- else
- return value;
+ return 0;
}
-void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
+static int dw_reg_write(void *context, unsigned int reg, unsigned int val)
{
- if (dev->flags & ACCESS_SWAP)
- b = swab32(b);
-
- if (dev->flags & ACCESS_16BIT) {
- writew_relaxed((u16)b, dev->base + offset);
- writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
- } else {
- writel_relaxed(b, dev->base + offset);
- }
+ struct dw_i2c_dev *dev = context;
+
+ writel_relaxed(val, dev->base + reg);
+
+ return 0;
+}
+
+static int dw_reg_read_swab(void *context, unsigned int reg, unsigned int *val)
+{
+ struct dw_i2c_dev *dev = context;
+
+ *val = swab32(readl_relaxed(dev->base + reg));
+
+ return 0;
+}
+
+static int dw_reg_write_swab(void *context, unsigned int reg, unsigned int val)
+{
+ struct dw_i2c_dev *dev = context;
+
+ writel_relaxed(swab32(val), dev->base + reg);
+
+ return 0;
+}
+
+static int dw_reg_read_word(void *context, unsigned int reg, unsigned int *val)
+{
+ struct dw_i2c_dev *dev = context;
+
+ *val = readw_relaxed(dev->base + reg) |
+ (readw_relaxed(dev->base + reg + 2) << 16);
+
+ return 0;
+}
+
+static int dw_reg_write_word(void *context, unsigned int reg, unsigned int val)
+{
+ struct dw_i2c_dev *dev = context;
+
+ writew_relaxed((u16)val, dev->base + reg);
+ writew_relaxed((u16)(val >> 16), dev->base + reg + 2);
+
+ return 0;
}
/**
- * i2c_dw_set_reg_access() - Set register access flags
+ * i2c_dw_init_regmap() - Initialize registers map
* @dev: device private data
+ * @base: Registers map base address
*
- * Autodetects needed register access mode and sets access flags accordingly.
- * This must be called before doing any other register access.
+ * Autodetects needed register access mode and creates the regmap with
+ * corresponding read/write callbacks. This must be called before doing any
+ * other register access.
*/
-int i2c_dw_set_reg_access(struct dw_i2c_dev *dev)
+int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
{
+ struct regmap_config map_cfg = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .disable_locking = true,
+ .reg_read = dw_reg_read,
+ .reg_write = dw_reg_write,
+ .max_register = DW_IC_COMP_TYPE
+ };
u32 reg;
int ret;
@@ -98,21 +137,33 @@ int i2c_dw_set_reg_access(struct dw_i2c_dev *dev)
if (ret)
return ret;
- reg = dw_readl(dev, DW_IC_COMP_TYPE);
+ reg = readl(dev->base + DW_IC_COMP_TYPE);
i2c_dw_release_lock(dev);
if (reg == swab32(DW_IC_COMP_TYPE_VALUE)) {
- /* Configure register endianness access */
- dev->flags |= ACCESS_SWAP;
+ map_cfg.reg_read = dw_reg_read_swab;
+ map_cfg.reg_write = dw_reg_write_swab;
} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
- /* Configure register access mode 16bit */
- dev->flags |= ACCESS_16BIT;
+ map_cfg.reg_read = dw_reg_read_word;
+ map_cfg.reg_write = dw_reg_write_word;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
dev_err(dev->dev,
"Unknown Synopsys component type: 0x%08x\n", reg);
return -ENODEV;
}
+ /*
+ * Note we'll check the return value of the regmap IO accessors only
+ * at the probe stage. The rest of the code won't do this because
+ * basically we have MMIO-based regmap so non of the read/write methods
+ * can fail.
+ */
+ dev->map = devm_regmap_init(dev->dev, NULL, dev, &map_cfg);
+ if (IS_ERR(dev->map)) {
+ dev_err(dev->dev, "Failed to init the registers map\n");
+ return PTR_ERR(dev->map);
+ }
+
return 0;
}
@@ -181,11 +232,17 @@ int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
return ret;
/* Configure SDA Hold Time if required */
- reg = dw_readl(dev, DW_IC_COMP_VERSION);
+ ret = regmap_read(dev->map, DW_IC_COMP_VERSION, ®);
+ if (ret)
+ goto err_release_lock;
+
if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
if (!dev->sda_hold_time) {
/* Keep previous hold time setting if no one set it */
- dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
+ ret = regmap_read(dev->map, DW_IC_SDA_HOLD,
+ &dev->sda_hold_time);
+ if (ret)
+ goto err_release_lock;
}
/*
@@ -209,14 +266,16 @@ int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev)
dev->sda_hold_time = 0;
}
+err_release_lock:
i2c_dw_release_lock(dev);
- return 0;
+ return ret;
}
void __i2c_dw_disable(struct dw_i2c_dev *dev)
{
int timeout = 100;
+ u32 status;
do {
__i2c_dw_disable_nowait(dev);
@@ -224,7 +283,8 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
* The enable status register may be unimplemented, but
* in that case this test reads zero and exits the loop.
*/
- if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == 0)
+ regmap_read(dev->map, DW_IC_ENABLE_STATUS, &status);
+ if ((status & 1) == 0)
return;
/*
@@ -303,22 +363,23 @@ void i2c_dw_release_lock(struct dw_i2c_dev *dev)
*/
int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
{
- int timeout = TIMEOUT;
+ u32 status;
+ int ret;
- while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
- if (timeout <= 0) {
- dev_warn(dev->dev, "timeout waiting for bus ready\n");
- i2c_recover_bus(&dev->adapter);
+ ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
+ !(status & DW_IC_STATUS_ACTIVITY),
+ 1100, 20000);
+ if (ret) {
+ dev_warn(dev->dev, "timeout waiting for bus ready\n");
- if (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY)
- return -ETIMEDOUT;
- return 0;
- }
- timeout--;
- usleep_range(1000, 1100);
+ i2c_recover_bus(&dev->adapter);
+
+ regmap_read(dev->map, DW_IC_STATUS, &status);
+ if (!(status & DW_IC_STATUS_ACTIVITY))
+ ret = 0;
}
- return 0;
+ return ret;
}
int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
@@ -344,15 +405,19 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
return -EIO;
}
-void i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
+int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
{
u32 param, tx_fifo_depth, rx_fifo_depth;
+ int ret;
/*
* Try to detect the FIFO depth if not set by interface driver,
* the depth could be from 2 to 256 from HW spec.
*/
- param = dw_readl(dev, DW_IC_COMP_PARAM_1);
+ ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, ¶m);
+ if (ret)
+ return ret;
+
tx_fifo_depth = ((param >> 16) & 0xff) + 1;
rx_fifo_depth = ((param >> 8) & 0xff) + 1;
if (!dev->tx_fifo_depth) {
@@ -364,6 +429,8 @@ void i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
rx_fifo_depth);
}
+
+ return 0;
}
u32 i2c_dw_func(struct i2c_adapter *adap)
@@ -375,17 +442,19 @@ u32 i2c_dw_func(struct i2c_adapter *adap)
void i2c_dw_disable(struct dw_i2c_dev *dev)
{
+ u32 dummy;
+
/* Disable controller */
__i2c_dw_disable(dev);
/* Disable all interrupts */
- dw_writel(dev, 0, DW_IC_INTR_MASK);
- dw_readl(dev, DW_IC_CLR_INTR);
+ regmap_write(dev->map, DW_IC_INTR_MASK, 0);
+ regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
}
void i2c_dw_disable_int(struct dw_i2c_dev *dev)
{
- dw_writel(dev, 0, DW_IC_INTR_MASK);
+ regmap_write(dev->map, DW_IC_INTR_MASK, 0);
}
MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index b220ad64c38d..b5b981c1bb0d 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -10,6 +10,7 @@
*/
#include <linux/i2c.h>
+#include <linux/regmap.h>
#define DW_IC_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C | \
I2C_FUNC_SMBUS_BYTE | \
@@ -120,8 +121,6 @@
#define STATUS_WRITE_IN_PROGRESS 0x1
#define STATUS_READ_IN_PROGRESS 0x2
-#define TIMEOUT 20 /* ms */
-
/*
* operation modes
*/
@@ -174,7 +173,9 @@
/**
* struct dw_i2c_dev - private i2c-designware data
* @dev: driver model device node
+ * @map: IO registers map
* @base: IO registers pointer
+ * @ext: Extended IO registers pointer
* @cmd_complete: tx completion indicator
* @clk: input reference clock
* @pclk: clock required to access the registers
@@ -224,6 +225,7 @@
*/
struct dw_i2c_dev {
struct device *dev;
+ struct regmap *map;
void __iomem *base;
void __iomem *ext;
struct completion cmd_complete;
@@ -276,8 +278,6 @@ struct dw_i2c_dev {
bool suspended;
};
-#define ACCESS_SWAP 0x00000001
-#define ACCESS_16BIT 0x00000002
#define ACCESS_INTR_MASK 0x00000004
#define ACCESS_NO_IRQ_SUSPEND 0x00000008
@@ -285,9 +285,7 @@ struct dw_i2c_dev {
#define MODEL_MSCC_OCELOT 0x00000200
#define MODEL_MASK 0x00000f00
-u32 dw_readl(struct dw_i2c_dev *dev, int offset);
-void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
-int i2c_dw_set_reg_access(struct dw_i2c_dev *dev);
+int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
int i2c_dw_set_sda_hold(struct dw_i2c_dev *dev);
@@ -297,19 +295,19 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
void i2c_dw_release_lock(struct dw_i2c_dev *dev);
int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
-void i2c_dw_set_fifo_size(struct dw_i2c_dev *dev);
+int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev);
u32 i2c_dw_func(struct i2c_adapter *adap);
void i2c_dw_disable(struct dw_i2c_dev *dev);
void i2c_dw_disable_int(struct dw_i2c_dev *dev);
static inline void __i2c_dw_enable(struct dw_i2c_dev *dev)
{
- dw_writel(dev, 1, DW_IC_ENABLE);
+ regmap_write(dev->map, DW_IC_ENABLE, 1);
}
static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev)
{
- dw_writel(dev, 0, DW_IC_ENABLE);
+ regmap_write(dev->map, DW_IC_ENABLE, 0);
}
void __i2c_dw_disable(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 3a58eef20936..f78cb0d08b4b 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -15,6 +15,7 @@
#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/regmap.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pm_runtime.h>
@@ -25,11 +26,11 @@
static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
{
/* Configure Tx/Rx FIFO threshold levels */
- dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
- dw_writel(dev, 0, DW_IC_RX_TL);
+ regmap_write(dev->map, DW_IC_TX_TL, dev->tx_fifo_depth / 2);
+ regmap_write(dev->map, DW_IC_RX_TL, 0);
/* Configure the I2C master */
- dw_writel(dev, dev->master_cfg, DW_IC_CON);
+ regmap_write(dev->map, DW_IC_CON, dev->master_cfg);
}
static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
@@ -44,8 +45,11 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
ret = i2c_dw_acquire_lock(dev);
if (ret)
return ret;
- comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
+
+ ret = regmap_read(dev->map, DW_IC_COMP_PARAM_1, &comp_param1);
i2c_dw_release_lock(dev);
+ if (ret)
+ return ret;
/* Set standard and fast speed dividers for high/low periods */
sda_falling_time = t->sda_fall_ns ?: 300; /* ns */
@@ -162,22 +166,22 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev)
__i2c_dw_disable(dev);
/* Write standard speed timing parameters */
- dw_writel(dev, dev->ss_hcnt, DW_IC_SS_SCL_HCNT);
- dw_writel(dev, dev->ss_lcnt, DW_IC_SS_SCL_LCNT);
+ regmap_write(dev->map, DW_IC_SS_SCL_HCNT, dev->ss_hcnt);
+ regmap_write(dev->map, DW_IC_SS_SCL_LCNT, dev->ss_lcnt);
/* Write fast mode/fast mode plus timing parameters */
- dw_writel(dev, dev->fs_hcnt, DW_IC_FS_SCL_HCNT);
- dw_writel(dev, dev->fs_lcnt, DW_IC_FS_SCL_LCNT);
+ regmap_write(dev->map, DW_IC_FS_SCL_HCNT, dev->fs_hcnt);
+ regmap_write(dev->map, DW_IC_FS_SCL_LCNT, dev->fs_lcnt);
/* Write high speed timing parameters if supported */
if (dev->hs_hcnt && dev->hs_lcnt) {
- dw_writel(dev, dev->hs_hcnt, DW_IC_HS_SCL_HCNT);
- dw_writel(dev, dev->hs_lcnt, DW_IC_HS_SCL_LCNT);
+ regmap_write(dev->map, DW_IC_HS_SCL_HCNT, dev->hs_hcnt);
+ regmap_write(dev->map, DW_IC_HS_SCL_LCNT, dev->hs_lcnt);
}
/* Write SDA hold time if supported */
if (dev->sda_hold_time)
- dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+ regmap_write(dev->map, DW_IC_SDA_HOLD, dev->sda_hold_time);
i2c_dw_configure_fifo_master(dev);
i2c_dw_release_lock(dev);
@@ -188,15 +192,15 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev)
static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
{
struct i2c_msg *msgs = dev->msgs;
- u32 ic_con, ic_tar = 0;
+ u32 ic_con = 0, ic_tar = 0;
+ u32 dummy;
/* Disable the adapter */
__i2c_dw_disable(dev);
/* If the slave address is ten bit address, enable 10BITADDR */
- ic_con = dw_readl(dev, DW_IC_CON);
if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
- ic_con |= DW_IC_CON_10BITADDR_MASTER;
+ ic_con = DW_IC_CON_10BITADDR_MASTER;
/*
* If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
* mode has to be enabled via bit 12 of IC_TAR register.
@@ -204,17 +208,17 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
* detected from registers.
*/
ic_tar = DW_IC_TAR_10BITADDR_MASTER;
- } else {
- ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
}
- dw_writel(dev, ic_con, DW_IC_CON);
+ regmap_update_bits(dev->map, DW_IC_CON, DW_IC_CON_10BITADDR_MASTER,
+ ic_con);
/*
* Set the slave (target) address and enable 10-bit addressing mode
* if applicable.
*/
- dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
+ regmap_write(dev->map, DW_IC_TAR,
+ msgs[dev->msg_write_idx].addr | ic_tar);
/* Enforce disabled interrupts (due to HW issues) */
i2c_dw_disable_int(dev);
@@ -223,11 +227,11 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
__i2c_dw_enable(dev);
/* Dummy read to avoid the register getting stuck on Bay Trail */
- dw_readl(dev, DW_IC_ENABLE_STATUS);
+ regmap_read(dev->map, DW_IC_ENABLE_STATUS, &dummy);
/* Clear and enable interrupts */
- dw_readl(dev, DW_IC_CLR_INTR);
- dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
+ regmap_read(dev->map, DW_IC_CLR_INTR, &dummy);
+ regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK);
}
/*
@@ -246,6 +250,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
u32 buf_len = dev->tx_buf_len;
u8 *buf = dev->tx_buf;
bool need_restart = false;
+ unsigned int flr;
intr_mask = DW_IC_INTR_MASTER_MASK;
@@ -278,8 +283,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
need_restart = true;
}
- tx_limit = dev->tx_fifo_depth - dw_readl(dev, DW_IC_TXFLR);
- rx_limit = dev->rx_fifo_depth - dw_readl(dev, DW_IC_RXFLR);
+ regmap_read(dev->map, DW_IC_TXFLR, &flr);
+ tx_limit = dev->tx_fifo_depth - flr;
+
+ regmap_read(dev->map, DW_IC_RXFLR, &flr);
+ rx_limit = dev->rx_fifo_depth - flr;
while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
u32 cmd = 0;
@@ -312,11 +320,14 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (dev->rx_outstanding >= dev->rx_fifo_depth)
break;
- dw_writel(dev, cmd | 0x100, DW_IC_DATA_CMD);
+ regmap_write(dev->map, DW_IC_DATA_CMD,
+ cmd | 0x100);
rx_limit--;
dev->rx_outstanding++;
- } else
- dw_writel(dev, cmd | *buf++, DW_IC_DATA_CMD);
+ } else {
+ regmap_write(dev->map, DW_IC_DATA_CMD,
+ cmd | *buf++);
+ }
tx_limit--; buf_len--;
}
@@ -346,7 +357,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
if (dev->msg_err)
intr_mask = 0;
- dw_writel(dev, intr_mask, DW_IC_INTR_MASK);
+ regmap_write(dev->map, DW_IC_INTR_MASK, intr_mask);
}
static u8
@@ -374,7 +385,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
int rx_valid;
for (; dev->msg_read_idx < dev->msgs_num; dev->msg_read_idx++) {
- u32 len;
+ u32 len, tmp;
u8 *buf;
if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
@@ -388,18 +399,18 @@ i2c_dw_read(struct dw_i2c_dev *dev)
buf = dev->rx_buf;
}
- rx_valid = dw_readl(dev, DW_IC_RXFLR);
+ regmap_read(dev->map, DW_IC_RXFLR, &rx_valid);
for (; len > 0 && rx_valid > 0; len--, rx_valid--) {
u32 flags = msgs[dev->msg_read_idx].flags;
- *buf = dw_readl(dev, DW_IC_DATA_CMD);
+ regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
/* Ensure length byte is a valid value */
if (flags & I2C_M_RECV_LEN &&
- *buf <= I2C_SMBUS_BLOCK_MAX && *buf > 0) {
- len = i2c_dw_recv_len(dev, *buf);
+ tmp <= I2C_SMBUS_BLOCK_MAX && tmp > 0) {
+ len = i2c_dw_recv_len(dev, tmp);
}
- buf++;
+ *buf++ = tmp;
dev->rx_outstanding--;
}
@@ -517,7 +528,7 @@ static const struct i2c_adapter_quirks i2c_dw_quirks = {
static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
{
- u32 stat;
+ u32 stat, dummy;
/*
* The IC_INTR_STAT register just indicates "enabled" interrupts.
@@ -525,47 +536,47 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
* in the IC_RAW_INTR_STAT register.
*
* That is,
- * stat = dw_readl(IC_INTR_STAT);
+ * stat = readl(IC_INTR_STAT);
* equals to,
- * stat = dw_readl(IC_RAW_INTR_STAT) & dw_readl(IC_INTR_MASK);
+ * stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK);
*
* The raw version might be useful for debugging purposes.
*/
- stat = dw_readl(dev, DW_IC_INTR_STAT);
+ regmap_read(dev->map, DW_IC_INTR_STAT, &stat);
/*
* Do not use the IC_CLR_INTR register to clear interrupts, or
* you'll miss some interrupts, triggered during the period from
- * dw_readl(IC_INTR_STAT) to dw_readl(IC_CLR_INTR).
+ * readl(IC_INTR_STAT) to readl(IC_CLR_INTR).
*
* Instead, use the separately-prepared IC_CLR_* registers.
*/
if (stat & DW_IC_INTR_RX_UNDER)
- dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &dummy);
if (stat & DW_IC_INTR_RX_OVER)
- dw_readl(dev, DW_IC_CLR_RX_OVER);
+ regmap_read(dev->map, DW_IC_CLR_RX_OVER, &dummy);
if (stat & DW_IC_INTR_TX_OVER)
- dw_readl(dev, DW_IC_CLR_TX_OVER);
+ regmap_read(dev->map, DW_IC_CLR_TX_OVER, &dummy);
if (stat & DW_IC_INTR_RD_REQ)
- dw_readl(dev, DW_IC_CLR_RD_REQ);
+ regmap_read(dev->map, DW_IC_CLR_RD_REQ, &dummy);
if (stat & DW_IC_INTR_TX_ABRT) {
/*
* The IC_TX_ABRT_SOURCE register is cleared whenever
* the IC_CLR_TX_ABRT is read. Preserve it beforehand.
*/
- dev->abort_source = dw_readl(dev, DW_IC_TX_ABRT_SOURCE);
- dw_readl(dev, DW_IC_CLR_TX_ABRT);
+ regmap_read(dev->map, DW_IC_TX_ABRT_SOURCE, &dev->abort_source);
+ regmap_read(dev->map, DW_IC_CLR_TX_ABRT, &dummy);
}
if (stat & DW_IC_INTR_RX_DONE)
- dw_readl(dev, DW_IC_CLR_RX_DONE);
+ regmap_read(dev->map, DW_IC_CLR_RX_DONE, &dummy);
if (stat & DW_IC_INTR_ACTIVITY)
- dw_readl(dev, DW_IC_CLR_ACTIVITY);
+ regmap_read(dev->map, DW_IC_CLR_ACTIVITY, &dummy);
if (stat & DW_IC_INTR_STOP_DET)
- dw_readl(dev, DW_IC_CLR_STOP_DET);
+ regmap_read(dev->map, DW_IC_CLR_STOP_DET, &dummy);
if (stat & DW_IC_INTR_START_DET)
- dw_readl(dev, DW_IC_CLR_START_DET);
+ regmap_read(dev->map, DW_IC_CLR_START_DET, &dummy);
if (stat & DW_IC_INTR_GEN_CALL)
- dw_readl(dev, DW_IC_CLR_GEN_CALL);
+ regmap_read(dev->map, DW_IC_CLR_GEN_CALL, &dummy);
return stat;
}
@@ -587,7 +598,7 @@ static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
* Anytime TX_ABRT is set, the contents of the tx/rx
* buffers are flushed. Make sure to skip them.
*/
- dw_writel(dev, 0, DW_IC_INTR_MASK);
+ regmap_write(dev->map, DW_IC_INTR_MASK, 0);
goto tx_aborted;
}
@@ -608,9 +619,9 @@ static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
complete(&dev->cmd_complete);
else if (unlikely(dev->flags & ACCESS_INTR_MASK)) {
/* Workaround to trigger pending interrupt */
- stat = dw_readl(dev, DW_IC_INTR_MASK);
+ regmap_read(dev->map, DW_IC_INTR_MASK, &stat);
i2c_dw_disable_int(dev);
- dw_writel(dev, stat, DW_IC_INTR_MASK);
+ regmap_write(dev->map, DW_IC_INTR_MASK, stat);
}
return 0;
@@ -621,8 +632,8 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
struct dw_i2c_dev *dev = dev_id;
u32 stat, enabled;
- enabled = dw_readl(dev, DW_IC_ENABLE);
- stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+ regmap_read(dev->map, DW_IC_ENABLE, &enabled);
+ regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat);
dev_dbg(dev->dev, "enabled=%#x stat=%#x\n", enabled, stat);
if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
return IRQ_NONE;
@@ -690,7 +701,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
dev->disable = i2c_dw_disable;
dev->disable_int = i2c_dw_disable_int;
- ret = i2c_dw_set_reg_access(dev);
+ ret = i2c_dw_init_regmap(dev);
if (ret)
return ret;
@@ -698,7 +709,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
if (ret)
return ret;
- i2c_dw_set_fifo_size(dev);
+ ret = i2c_dw_set_fifo_size(dev);
+ if (ret)
+ return ret;
ret = dev->init(dev);
if (ret)
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index f5ecf76c0d02..9f8aef7a69b2 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -11,6 +11,7 @@
#include <linux/errno.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/regmap.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/pm_runtime.h>
@@ -20,12 +21,12 @@
static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
{
/* Configure Tx/Rx FIFO threshold levels. */
- dw_writel(dev, 0, DW_IC_TX_TL);
- dw_writel(dev, 0, DW_IC_RX_TL);
+ regmap_write(dev->map, DW_IC_TX_TL, 0);
+ regmap_write(dev->map, DW_IC_RX_TL, 0);
/* Configure the I2C slave. */
- dw_writel(dev, dev->slave_cfg, DW_IC_CON);
- dw_writel(dev, DW_IC_INTR_SLAVE_MASK, DW_IC_INTR_MASK);
+ regmap_write(dev->map, DW_IC_CON, dev->slave_cfg);
+ regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_SLAVE_MASK);
}
/**
@@ -49,7 +50,7 @@ static int i2c_dw_init_slave(struct dw_i2c_dev *dev)
/* Write SDA hold time if supported */
if (dev->sda_hold_time)
- dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+ regmap_write(dev->map, DW_IC_SDA_HOLD, dev->sda_hold_time);
i2c_dw_configure_fifo_slave(dev);
i2c_dw_release_lock(dev);
@@ -72,7 +73,7 @@ static int i2c_dw_reg_slave(struct i2c_client *slave)
* the address to which the DW_apb_i2c responds.
*/
__i2c_dw_disable_nowait(dev);
- dw_writel(dev, slave->addr, DW_IC_SAR);
+ regmap_write(dev->map, DW_IC_SAR, slave->addr);
dev->slave = slave;
__i2c_dw_enable(dev);
@@ -103,7 +104,7 @@ static int i2c_dw_unreg_slave(struct i2c_client *slave)
static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
{
- u32 stat;
+ u32 stat, dummy;
/*
* The IC_INTR_STAT register just indicates "enabled" interrupts.
@@ -111,39 +112,39 @@ static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
* in the IC_RAW_INTR_STAT register.
*
* That is,
- * stat = dw_readl(IC_INTR_STAT);
+ * stat = readl(IC_INTR_STAT);
* equals to,
- * stat = dw_readl(IC_RAW_INTR_STAT) & dw_readl(IC_INTR_MASK);
+ * stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK);
*
* The raw version might be useful for debugging purposes.
*/
- stat = dw_readl(dev, DW_IC_INTR_STAT);
+ regmap_read(dev->map, DW_IC_INTR_STAT, &stat);
/*
* Do not use the IC_CLR_INTR register to clear interrupts, or
* you'll miss some interrupts, triggered during the period from
- * dw_readl(IC_INTR_STAT) to dw_readl(IC_CLR_INTR).
+ * readl(IC_INTR_STAT) to readl(IC_CLR_INTR).
*
* Instead, use the separately-prepared IC_CLR_* registers.
*/
if (stat & DW_IC_INTR_TX_ABRT)
- dw_readl(dev, DW_IC_CLR_TX_ABRT);
+ regmap_read(dev->map, DW_IC_CLR_TX_ABRT, &dummy);
if (stat & DW_IC_INTR_RX_UNDER)
- dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &dummy);
if (stat & DW_IC_INTR_RX_OVER)
- dw_readl(dev, DW_IC_CLR_RX_OVER);
+ regmap_read(dev->map, DW_IC_CLR_RX_OVER, &dummy);
if (stat & DW_IC_INTR_TX_OVER)
- dw_readl(dev, DW_IC_CLR_TX_OVER);
+ regmap_read(dev->map, DW_IC_CLR_TX_OVER, &dummy);
if (stat & DW_IC_INTR_RX_DONE)
- dw_readl(dev, DW_IC_CLR_RX_DONE);
+ regmap_read(dev->map, DW_IC_CLR_RX_DONE, &dummy);
if (stat & DW_IC_INTR_ACTIVITY)
- dw_readl(dev, DW_IC_CLR_ACTIVITY);
+ regmap_read(dev->map, DW_IC_CLR_ACTIVITY, &dummy);
if (stat & DW_IC_INTR_STOP_DET)
- dw_readl(dev, DW_IC_CLR_STOP_DET);
+ regmap_read(dev->map, DW_IC_CLR_STOP_DET, &dummy);
if (stat & DW_IC_INTR_START_DET)
- dw_readl(dev, DW_IC_CLR_START_DET);
+ regmap_read(dev->map, DW_IC_CLR_START_DET, &dummy);
if (stat & DW_IC_INTR_GEN_CALL)
- dw_readl(dev, DW_IC_CLR_GEN_CALL);
+ regmap_read(dev->map, DW_IC_CLR_GEN_CALL, &dummy);
return stat;
}
@@ -155,14 +156,14 @@ static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
{
- u32 raw_stat, stat, enabled;
- u8 val, slave_activity;
+ u32 raw_stat, stat, enabled, tmp;
+ u8 val = 0, slave_activity;
- stat = dw_readl(dev, DW_IC_INTR_STAT);
- enabled = dw_readl(dev, DW_IC_ENABLE);
- raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
- slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
- DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
+ regmap_read(dev->map, DW_IC_INTR_STAT, &stat);
+ regmap_read(dev->map, DW_IC_ENABLE, &enabled);
+ regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &raw_stat);
+ regmap_read(dev->map, DW_IC_STATUS, &tmp);
+ slave_activity = ((tmp & DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY) || !dev->slave)
return 0;
@@ -177,7 +178,8 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
if (stat & DW_IC_INTR_RD_REQ) {
if (slave_activity) {
if (stat & DW_IC_INTR_RX_FULL) {
- val = dw_readl(dev, DW_IC_DATA_CMD);
+ regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
+ val = tmp;
if (!i2c_slave_event(dev->slave,
I2C_SLAVE_WRITE_RECEIVED,
@@ -185,24 +187,24 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
dev_vdbg(dev->dev, "Byte %X acked!",
val);
}
- dw_readl(dev, DW_IC_CLR_RD_REQ);
+ regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
stat = i2c_dw_read_clear_intrbits_slave(dev);
} else {
- dw_readl(dev, DW_IC_CLR_RD_REQ);
- dw_readl(dev, DW_IC_CLR_RX_UNDER);
+ regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);
+ regmap_read(dev->map, DW_IC_CLR_RX_UNDER, &tmp);
stat = i2c_dw_read_clear_intrbits_slave(dev);
}
if (!i2c_slave_event(dev->slave,
I2C_SLAVE_READ_REQUESTED,
&val))
- dw_writel(dev, val, DW_IC_DATA_CMD);
+ regmap_write(dev->map, DW_IC_DATA_CMD, val);
}
}
if (stat & DW_IC_INTR_RX_DONE) {
if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
&val))
- dw_readl(dev, DW_IC_CLR_RX_DONE);
+ regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
stat = i2c_dw_read_clear_intrbits_slave(dev);
@@ -210,7 +212,8 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
}
if (stat & DW_IC_INTR_RX_FULL) {
- val = dw_readl(dev, DW_IC_DATA_CMD);
+ regmap_read(dev->map, DW_IC_DATA_CMD, &tmp);
+ val = tmp;
if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
&val))
dev_vdbg(dev->dev, "Byte %X acked!", val);
@@ -252,7 +255,7 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
dev->disable = i2c_dw_disable;
dev->disable_int = i2c_dw_disable_int;
- ret = i2c_dw_set_reg_access(dev);
+ ret = i2c_dw_init_regmap(dev);
if (ret)
return ret;
@@ -260,7 +263,9 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
if (ret)
return ret;
- i2c_dw_set_fifo_size(dev);
+ ret = i2c_dw_set_fifo_size(dev);
+ if (ret)
+ return ret;
ret = dev->init(dev);
if (ret)
--
2.25.1
Seeing the DW I2C platform driver is getting overcomplicated with a lot of
vendor-specific configs let's introduce a glue-layer interface so new
platforms which equipped with Synopsys Designware APB I2C IP-core would
be able to handle their peculiarities in the dedicated objects.
The generic platform setups and cleanups can now be performed by means of
two new functions exported from the Dw I2C platform driver:
int i2c_dw_plat_setup(struct dw_i2c_dev *dev);
int i2c_dw_plat_clear(struct dw_i2c_dev *dev);
They also install and remove the I2C controller respectively. In addition
if device supports the PM interface a glue driver can use the generic
platform PM callbacks collected into the PM dev ops structure:
const struct dev_pm_ops i2c_dw_plat_dev_pm_ops;
Before setting the platform DW I2C device up the glue probe code is
supposed to create an instance of `struct dw_i2c_dev` and pre-initialize
its `struct device` pointer together with optional platform-specific
flags. Currently the ACCESS_NO_IRQ_SUSPEND and ACCESS_INTR_MASK flags are
supported.
Note we moved the platform driver private data setup to the generic
platform probe method. By doing so the driver data pointer will be free
to be used by the glue-layer driver.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/i2c-designware-core.h | 4 +
drivers/i2c/busses/i2c-designware-platdrv.c | 84 +++++++++++++--------
drivers/i2c/busses/i2c-designware-platdrv.h | 16 ++++
3 files changed, 71 insertions(+), 33 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-designware-platdrv.h
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index b5b981c1bb0d..10606266b30c 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -8,6 +8,8 @@
* Copyright (C) 2007 MontaVista Software Inc.
* Copyright (C) 2009 Provigent Ltd.
*/
+#ifndef __I2C_DESIGNWARE_CORE_H__
+#define __I2C_DESIGNWARE_CORE_H__
#include <linux/i2c.h>
#include <linux/regmap.h>
@@ -324,3 +326,5 @@ extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev);
#else
static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) { return 0; }
#endif
+
+#endif /* __I2C_DESIGNWARE_CORE_H__ */
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 5536673060cc..274953155569 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -32,6 +32,7 @@
#include <linux/suspend.h>
#include "i2c-designware-core.h"
+#include "i2c-designware-platdrv.h"
static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
{
@@ -80,9 +81,9 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],
kfree(buf.pointer);
}
-static int dw_i2c_acpi_configure(struct platform_device *pdev)
+static int dw_i2c_acpi_configure(struct dw_i2c_dev *dev)
{
- struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+ struct platform_device *pdev = to_platform_device(dev->dev);
struct i2c_timings *t = &dev->timings;
u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0;
@@ -135,7 +136,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
};
MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
#else
-static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
+static inline int dw_i2c_acpi_configure(struct dw_i2c_dev *dev)
{
return -ENODEV;
}
@@ -154,9 +155,9 @@ static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
return 0;
}
-static int dw_i2c_of_configure(struct platform_device *pdev)
+static int dw_i2c_of_configure(struct dw_i2c_dev *dev)
{
- struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+ struct platform_device *pdev = to_platform_device(dev->dev);
struct resource *mem;
switch (dev->flags & MODEL_MASK) {
@@ -180,7 +181,7 @@ static const struct of_device_id dw_i2c_of_match[] = {
};
MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
#else
-static inline int dw_i2c_of_configure(struct platform_device *pdev)
+static inline int dw_i2c_of_configure(struct dw_i2c_dev *dev)
{
return -ENODEV;
}
@@ -234,33 +235,25 @@ static const u32 supported_speeds[] = {
I2C_MAX_STANDARD_MODE_FREQ,
};
-static int dw_i2c_plat_probe(struct platform_device *pdev)
+int i2c_dw_plat_setup(struct dw_i2c_dev *dev)
{
+ struct platform_device *pdev = to_platform_device(dev->dev);
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
struct i2c_adapter *adap;
- struct dw_i2c_dev *dev;
struct i2c_timings *t;
u32 acpi_speed;
struct resource *mem;
- int i, irq, ret;
+ int i, ret;
- irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
-
- dev = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
- if (!dev)
- return -ENOMEM;
+ dev->irq = platform_get_irq(pdev, 0);
+ if (dev->irq < 0)
+ return dev->irq;
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(&pdev->dev, mem);
if (IS_ERR(dev->base))
return PTR_ERR(dev->base);
- dev->dev = &pdev->dev;
- dev->irq = irq;
- platform_set_drvdata(pdev, dev);
-
dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
if (IS_ERR(dev->rst))
return PTR_ERR(dev->rst);
@@ -295,13 +288,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
else
t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ;
- dev->flags |= (uintptr_t)device_get_match_data(&pdev->dev);
-
if (pdev->dev.of_node)
- dw_i2c_of_configure(pdev);
+ dw_i2c_of_configure(dev);
if (has_acpi_companion(&pdev->dev))
- dw_i2c_acpi_configure(pdev);
+ dw_i2c_acpi_configure(dev);
/*
* Only standard mode at 100kHz, fast mode at 400kHz,
@@ -393,10 +384,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
reset_control_assert(dev->rst);
return ret;
}
+EXPORT_SYMBOL_GPL(i2c_dw_plat_setup);
-static int dw_i2c_plat_remove(struct platform_device *pdev)
+int i2c_dw_plat_clear(struct dw_i2c_dev *dev)
{
- struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+ struct platform_device *pdev = to_platform_device(dev->dev);
pm_runtime_get_sync(&pdev->dev);
@@ -412,6 +404,29 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
return 0;
}
+EXPORT_SYMBOL_GPL(i2c_dw_plat_clear);
+
+static int dw_i2c_plat_probe(struct platform_device *pdev)
+{
+ struct dw_i2c_dev *dev;
+
+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+ if (!dev)
+ return -ENOMEM;
+
+ dev->dev = &pdev->dev;
+ dev->flags |= (uintptr_t)device_get_match_data(dev->dev);
+ platform_set_drvdata(pdev, dev);
+
+ return i2c_dw_plat_setup(dev);
+}
+
+static int dw_i2c_plat_remove(struct platform_device *pdev)
+{
+ struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+ return i2c_dw_plat_clear(dev);
+}
#ifdef CONFIG_PM_SLEEP
static int dw_i2c_plat_prepare(struct device *dev)
@@ -470,17 +485,20 @@ static int dw_i2c_plat_resume(struct device *dev)
return 0;
}
-static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
+#else
+
+#define dw_i2c_plat_prepare NULL
+#define dw_i2c_plat_complete NULL
+
+#endif
+
+const struct dev_pm_ops i2c_dw_plat_dev_pm_ops = {
.prepare = dw_i2c_plat_prepare,
.complete = dw_i2c_plat_complete,
SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
};
-
-#define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-#else
-#define DW_I2C_DEV_PMOPS NULL
-#endif
+EXPORT_SYMBOL_GPL(i2c_dw_plat_dev_pm_ops);
/* Work with hotplug and coldplug */
MODULE_ALIAS("platform:i2c_designware");
@@ -492,7 +510,7 @@ static struct platform_driver dw_i2c_driver = {
.name = "i2c_designware",
.of_match_table = of_match_ptr(dw_i2c_of_match),
.acpi_match_table = ACPI_PTR(dw_i2c_acpi_match),
- .pm = DW_I2C_DEV_PMOPS,
+ .pm = &i2c_dw_plat_dev_pm_ops,
},
};
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.h b/drivers/i2c/busses/i2c-designware-platdrv.h
new file mode 100644
index 000000000000..8916c4f61d7f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-platdrv.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Synopsys DesignWare I2C adapter driver.
+ */
+#ifndef __I2C_DESIGNWARE_PLATDRV_H__
+#define __I2C_DESIGNWARE_PLATDRV_H__
+
+#include <linux/pm.h>
+
+#include "i2c-designware-core.h"
+
+extern int i2c_dw_plat_setup(struct dw_i2c_dev *dev);
+extern int i2c_dw_plat_clear(struct dw_i2c_dev *dev);
+extern const struct dev_pm_ops i2c_dw_plat_dev_pm_ops;
+
+#endif /* __I2C_DESIGNWARE_PLATDRV_H__ */
--
2.25.1
A PM workaround activated by the flag MODEL_CHERRYTRAIL has been removed
since commit 9cbeeca05049 ("i2c: designware: Remove Cherry Trail PMIC I2C
bus pm_disabled workaround"), but the flag most likely by mistake has been
left in the Dw I2C drivers. Lets remove it.
By doing so we get rid from the last DW APB I2C IP-core model flag, so we
can remove the MODEL_MASK macro too.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/i2c-designware-core.h | 3 ---
drivers/i2c/busses/i2c-designware-pcidrv.c | 1 -
drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 64544777a1fa..5a1f6b623a9a 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -281,9 +281,6 @@ struct dw_i2c_dev {
#define ACCESS_INTR_MASK 0x00000004
#define ACCESS_NO_IRQ_SUSPEND 0x00000008
-#define MODEL_CHERRYTRAIL 0x00000100
-#define MODEL_MASK 0x00000f00
-
int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7a0b65b5b5b5..76357b575aa5 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -166,7 +166,6 @@ static struct dw_pci_controller dw_pci_controllers[] = {
.tx_fifo_depth = 32,
.rx_fifo_depth = 32,
.functionality = I2C_FUNC_10BIT_ADDR,
- .flags = MODEL_CHERRYTRAIL,
.scl_sda_cfg = &byt_config,
},
[elkhartlake] = {
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 1f56865bb6ca..f577e2a92a4f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -124,7 +124,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
{ "INT3432", 0 },
{ "INT3433", 0 },
{ "80860F41", ACCESS_NO_IRQ_SUSPEND },
- { "808622C1", ACCESS_NO_IRQ_SUSPEND | MODEL_CHERRYTRAIL },
+ { "808622C1", ACCESS_NO_IRQ_SUSPEND },
{ "AMD0010", ACCESS_INTR_MASK },
{ "AMDI0010", ACCESS_INTR_MASK },
{ "AMDI0510", 0 },
--
2.25.1
Since glue-layer drivers design is now supported by the DW APB I2C
platform code lets unpin MSCC Ocelot I2C driver at least. It won't be that
hard because the only difference between this controller and vanilly core
is in what the former sets the sda hold time in a dedicated configure
registers space.
Note I enabled the new driver by default for the MSCC Ocelot platform so
one would be automatically built and we wouldn't need to alter the in- and
out-of-source tree platform configs.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/Kconfig | 12 +++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-designware-core.h | 3 -
drivers/i2c/busses/i2c-designware-mscc.c | 83 +++++++++++++++++++++
drivers/i2c/busses/i2c-designware-platdrv.c | 40 ----------
5 files changed, 96 insertions(+), 43 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-designware-mscc.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index ed6927c4c540..2f047cf07fee 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -566,6 +566,18 @@ config I2C_DESIGNWARE_SLAVE
This is not a standalone module, this module compiles together with
i2c-designware-core.
+config I2C_DESIGNWARE_MSCC
+ tristate "Microsemi Ocelot I2C"
+ depends on MSCC_OCELOT
+ select I2C_DESIGNWARE_PLATFORM
+ default y
+ help
+ This driver supports the Microsemi Ocelot SoC version of the Synopsys
+ Designware I2C IP-core.
+
+ The driver can also be built as a module. If so, the module will be
+ called i2c-designware-mscc.
+
config I2C_DESIGNWARE_PCI
tristate "Synopsys DesignWare PCI"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index c6813d7b2780..480a9fe4fb64 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -55,6 +55,7 @@ i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-slave.o
obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-y := i2c-designware-platdrv.o
i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
+obj-$(CONFIG_I2C_DESIGNWARE_MSCC) += i2c-designware-mscc.o
obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o
i2c-designware-pci-y := i2c-designware-pcidrv.o
obj-$(CONFIG_I2C_DIGICOLOR) += i2c-digicolor.o
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 10606266b30c..64544777a1fa 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -177,7 +177,6 @@
* @dev: driver model device node
* @map: IO registers map
* @base: IO registers pointer
- * @ext: Extended IO registers pointer
* @cmd_complete: tx completion indicator
* @clk: input reference clock
* @pclk: clock required to access the registers
@@ -229,7 +228,6 @@ struct dw_i2c_dev {
struct device *dev;
struct regmap *map;
void __iomem *base;
- void __iomem *ext;
struct completion cmd_complete;
struct clk *clk;
struct clk *pclk;
@@ -284,7 +282,6 @@ struct dw_i2c_dev {
#define ACCESS_NO_IRQ_SUSPEND 0x00000008
#define MODEL_CHERRYTRAIL 0x00000100
-#define MODEL_MSCC_OCELOT 0x00000200
#define MODEL_MASK 0x00000f00
int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-mscc.c b/drivers/i2c/busses/i2c-designware-mscc.c
new file mode 100644
index 000000000000..0649e3d1fefc
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-mscc.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Microsemi Ocelot I2C Controller.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+#include "i2c-designware-core.h"
+#include "i2c-designware-platdrv.h"
+
+#define MSCC_ICPU_CFG_TWI_DELAY 0x0
+#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE BIT(0)
+#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER 0x4
+
+struct mscc_i2c_dev {
+ struct dw_i2c_dev dev;
+ void __iomem *ext;
+};
+#define to_mscc_device(_dev) container_of((_dev), struct mscc_i2c_dev, dev)
+
+static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
+{
+ struct mscc_i2c_dev *mscc = to_mscc_device(dev);
+
+ writel((dev->sda_hold_time << 1) | MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
+ mscc->ext + MSCC_ICPU_CFG_TWI_DELAY);
+
+ return 0;
+}
+
+static int mscc_i2c_plat_probe(struct platform_device *pdev)
+{
+ struct mscc_i2c_dev *mscc;
+ struct resource *mem;
+
+ mscc = devm_kzalloc(&pdev->dev, sizeof(*mscc), GFP_KERNEL);
+ if (!mscc)
+ return -ENOMEM;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ mscc->ext = devm_ioremap_resource(&pdev->dev, mem);
+ if (!IS_ERR(mscc->ext))
+ mscc->dev.set_sda_hold_time = mscc_twi_set_sda_hold_time;
+
+ mscc->dev.dev = &pdev->dev;
+ platform_set_drvdata(pdev, mscc);
+
+ return i2c_dw_plat_setup(&mscc->dev);
+}
+
+static int mscc_i2c_plat_remove(struct platform_device *pdev)
+{
+ struct mscc_i2c_dev *mscc = platform_get_drvdata(pdev);
+
+ return i2c_dw_plat_clear(&mscc->dev);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id mscc_i2c_of_match[] = {
+ { .compatible = "mscc,ocelot-i2c" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mscc_i2c_of_match);
+#endif
+
+static struct platform_driver mscc_i2c_driver = {
+ .probe = mscc_i2c_plat_probe,
+ .remove = mscc_i2c_plat_remove,
+ .driver = {
+ .name = "i2c_ocelot",
+ .of_match_table = of_match_ptr(mscc_i2c_of_match),
+ .pm = &i2c_dw_plat_dev_pm_ops,
+ },
+};
+module_platform_driver(mscc_i2c_driver);
+
+MODULE_AUTHOR("Alexandre Belloni <[email protected]>");
+MODULE_DESCRIPTION("Microsemi Ocelot I2C Controller");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 274953155569..1f56865bb6ca 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -143,48 +143,11 @@ static inline int dw_i2c_acpi_configure(struct dw_i2c_dev *dev)
#endif
#ifdef CONFIG_OF
-#define MSCC_ICPU_CFG_TWI_DELAY 0x0
-#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE BIT(0)
-#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER 0x4
-
-static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
-{
- writel((dev->sda_hold_time << 1) | MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
- dev->ext + MSCC_ICPU_CFG_TWI_DELAY);
-
- return 0;
-}
-
-static int dw_i2c_of_configure(struct dw_i2c_dev *dev)
-{
- struct platform_device *pdev = to_platform_device(dev->dev);
- struct resource *mem;
-
- switch (dev->flags & MODEL_MASK) {
- case MODEL_MSCC_OCELOT:
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- dev->ext = devm_ioremap_resource(&pdev->dev, mem);
- if (!IS_ERR(dev->ext))
- dev->set_sda_hold_time = mscc_twi_set_sda_hold_time;
- break;
- default:
- break;
- }
-
- return 0;
-}
-
static const struct of_device_id dw_i2c_of_match[] = {
{ .compatible = "snps,designware-i2c", },
- { .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
{},
};
MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
-#else
-static inline int dw_i2c_of_configure(struct dw_i2c_dev *dev)
-{
- return -ENODEV;
-}
#endif
static void i2c_dw_configure_master(struct dw_i2c_dev *dev)
@@ -288,9 +251,6 @@ int i2c_dw_plat_setup(struct dw_i2c_dev *dev)
else
t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ;
- if (pdev->dev.of_node)
- dw_i2c_of_configure(dev);
-
if (has_acpi_companion(&pdev->dev))
dw_i2c_acpi_configure(dev);
--
2.25.1
This is a preparation patch before adding a glue platform driver for the
Baikal-T1 I2C controller. Since the i2c controller registers are indirectly
accessed by means of the Baikal-T1 System Controller registers we need to
have a way to disable the default registers mapping setup procedure and
make the DW I2C core/platform code to use a provided by a glue driver
regmap.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/i2c-designware-common.c | 7 +++++++
drivers/i2c/busses/i2c-designware-platdrv.c | 14 ++++++++++----
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 35c5ad7e274e..141ea0651a8f 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -133,6 +133,13 @@ int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
u32 reg;
int ret;
+ /*
+ * Skip detecting the registers map configuration if the regmap has
+ * already been provided by a higher code.
+ */
+ if (dev->map)
+ return 0;
+
ret = i2c_dw_acquire_lock(dev);
if (ret)
return ret;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index f577e2a92a4f..9d131a64ea81 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -212,10 +212,16 @@ int i2c_dw_plat_setup(struct dw_i2c_dev *dev)
if (dev->irq < 0)
return dev->irq;
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- dev->base = devm_ioremap_resource(&pdev->dev, mem);
- if (IS_ERR(dev->base))
- return PTR_ERR(dev->base);
+ /*
+ * Don't try to get the controller registers MMIO space if regmap has
+ * been provided by a higher level code.
+ */
+ if (!dev->map) {
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ dev->base = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(dev->base))
+ return PTR_ERR(dev->base);
+ }
dev->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
if (IS_ERR(dev->rst))
--
2.25.1
Baikal-T1 System Controller is equipped with a dedicated I2C Controller
which functionality is based on the DW APB I2C IP-core, the only
difference in a way it' registers are accessed. There are three access
register provided in the System Controller registers map, which
indirectly address the normal DW APB I2C registers space.
So in order to have the Baikal-T1 System I2C Controller supported by the
common DW APB I2C driver we created a dedicated glue driver, which
retrieves the syscon regmap from the parental dt node and creates a
new regmap based on it. The new regmap is then passed to the generic DW I2C
platform driver initializer.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-designware-bt1.c | 129 ++++++++++++++++++++++++
3 files changed, 141 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-designware-bt1.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 2f047cf07fee..d4a5d78cc181 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -578,6 +578,17 @@ config I2C_DESIGNWARE_MSCC
The driver can also be built as a module. If so, the module will be
called i2c-designware-mscc.
+config I2C_DESIGNWARE_BT1
+ tristate "Baikal-T1 System I2C"
+ depends on (MIPS_BAIKAL_T1 && OF) || COMPILE_TEST
+ select I2C_DESIGNWARE_PLATFORM
+ help
+ This driver supports the Baikal-T1 SoC version of the Synopsys
+ Designware I2C IP-core.
+
+ The driver can also be built as a module. If so, the module will be
+ called i2c-designware-bt1.
+
config I2C_DESIGNWARE_PCI
tristate "Synopsys DesignWare PCI"
depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 480a9fe4fb64..7b044d4e299a 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM) += i2c-designware-platform.o
i2c-designware-platform-y := i2c-designware-platdrv.o
i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
obj-$(CONFIG_I2C_DESIGNWARE_MSCC) += i2c-designware-mscc.o
+obj-$(CONFIG_I2C_DESIGNWARE_BT1) += i2c-designware-bt1.o
obj-$(CONFIG_I2C_DESIGNWARE_PCI) += i2c-designware-pci.o
i2c-designware-pci-y := i2c-designware-pcidrv.o
obj-$(CONFIG_I2C_DIGICOLOR) += i2c-digicolor.o
diff --git a/drivers/i2c/busses/i2c-designware-bt1.c b/drivers/i2c/busses/i2c-designware-bt1.c
new file mode 100644
index 000000000000..ed157d1c3b81
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-bt1.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ *
+ * Authors:
+ * Serge Semin <[email protected]>
+ *
+ * Baikal-T1 System I2C driver
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+#include "i2c-designware-core.h"
+#include "i2c-designware-platdrv.h"
+
+/*
+ * Access registers to the normal I2C regspace.
+ */
+#define BT1_I2C_CTL 0x100
+#define BT1_I2C_CTL_ADDR_MASK GENMASK(7, 0)
+#define BT1_I2C_CTL_WR BIT(8)
+#define BT1_I2C_CTL_GO BIT(31)
+#define BT1_I2C_DI 0x104
+#define BT1_I2C_DO 0x108
+
+struct bt1_i2c_dev {
+ struct dw_i2c_dev dev;
+ struct regmap *sys_regs;
+};
+
+static int bt1_i2c_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct bt1_i2c_dev *bt1 = context;
+ int ret;
+
+ /*
+ * Note these methods shouldn't ever fail because the system controller
+ * registers are memory mapped. We check the return value just in case.
+ */
+ ret = regmap_write(bt1->sys_regs, BT1_I2C_CTL,
+ BT1_I2C_CTL_GO | (reg & BT1_I2C_CTL_ADDR_MASK));
+ if (ret)
+ return ret;
+
+ return regmap_read(bt1->sys_regs, BT1_I2C_DO, val);
+}
+
+static int bt1_i2c_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct bt1_i2c_dev *bt1 = context;
+ int ret;
+
+ ret = regmap_write(bt1->sys_regs, BT1_I2C_DI, val);
+ if (ret)
+ return ret;
+
+ return regmap_write(bt1->sys_regs, BT1_I2C_CTL,
+ BT1_I2C_CTL_GO | BT1_I2C_CTL_WR | (reg & BT1_I2C_CTL_ADDR_MASK));
+}
+
+static struct regmap_config bt1_i2c_cfg = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .fast_io = true,
+ .reg_read = bt1_i2c_read,
+ .reg_write = bt1_i2c_write,
+ .max_register = DW_IC_COMP_TYPE
+};
+
+static int bt1_i2c_plat_probe(struct platform_device *pdev)
+{
+ struct bt1_i2c_dev *bt1;
+
+ bt1 = devm_kzalloc(&pdev->dev, sizeof(*bt1), GFP_KERNEL);
+ if (!bt1)
+ return -ENOMEM;
+
+ bt1->sys_regs = syscon_node_to_regmap(pdev->dev.of_node->parent);
+ if (IS_ERR(bt1->sys_regs)) {
+ dev_err(&pdev->dev, "Couldn't get BT1 I2C register map\n");
+ return PTR_ERR(bt1->sys_regs);
+ }
+
+ bt1->dev.map = devm_regmap_init(&pdev->dev, NULL, bt1, &bt1_i2c_cfg);
+ if (IS_ERR(bt1->dev.map)) {
+ dev_err(&pdev->dev, "Failed to init the registers map\n");
+ return PTR_ERR(bt1->dev.map);
+ }
+
+ bt1->dev.dev = &pdev->dev;
+ platform_set_drvdata(pdev, bt1);
+
+ return i2c_dw_plat_setup(&bt1->dev);
+}
+
+static int bt1_i2c_plat_remove(struct platform_device *pdev)
+{
+ struct bt1_i2c_dev *bt1 = platform_get_drvdata(pdev);
+
+ return i2c_dw_plat_clear(&bt1->dev);
+}
+
+static const struct of_device_id bt1_i2c_of_match[] = {
+ { .compatible = "baikal,bt1-sys-i2c" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, bt1_i2c_of_match);
+
+static struct platform_driver bt1_i2c_driver = {
+ .probe = bt1_i2c_plat_probe,
+ .remove = bt1_i2c_plat_remove,
+ .driver = {
+ .name = "bt1-sys-i2c",
+ .of_match_table = bt1_i2c_of_match,
+ .pm = &i2c_dw_plat_dev_pm_ops,
+ },
+};
+module_platform_driver(bt1_i2c_driver);
+
+MODULE_AUTHOR("Serge Semin <[email protected]>");
+MODULE_DESCRIPTION("Baikal-T1 System I2C Controller");
+MODULE_LICENSE("GPL v2");
--
2.25.1
DW APB I2C slave code in fact depends on the DW I2C driver core, but not
on the platform code. Yes, the I2C slave interface is currently supported
by the platform version of the IP core, but it doesn't make it dependent
on it. So make sure the DW APB I2C slave config is only available if the
I2C_DESIGNWARE_CORE config is enabled.
Signed-off-by: Serge Semin <[email protected]>
Cc: Alexey Malahov <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Paul Burton <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Wolfram Sang <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/i2c/busses/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 14368c46cb63..368aa64e9266 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -541,8 +541,8 @@ config I2C_DESIGNWARE_PLATFORM
config I2C_DESIGNWARE_SLAVE
bool "Synopsys DesignWare Slave"
+ depends on I2C_DESIGNWARE_CORE
select I2C_SLAVE
- depends on I2C_DESIGNWARE_PLATFORM
help
If you say yes to this option, support will be included for the
Synopsys DesignWare I2C slave adapter.
--
2.25.1
On Sun, May 10, 2020 at 12:50:08PM +0300, Serge Semin wrote:
> Modern device tree bindings are supposed to be created as YAML-files
> in accordance with dt-schema. This commit replaces Synopsys DW I2C
> legacy bare text bindings with YAML file. As before the bindings file
> states that the corresponding dts node is supposed to be compatible
> either with generic DW I2C controller or with Microsemi Ocelot SoC I2C
> one, to have registers, interrupts and clocks properties. In addition
> the node may have clock-frequency, i2c-sda-hold-time-ns,
> i2c-scl-falling-time-ns and i2c-sda-falling-time-ns optional properties.
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Mika Westerberg <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
>
> ---
>
> Changelog v2:
> - Make sure that "mscc,ocelot-i2c" compatible node may have up to two
> registers space defined, while normal DW I2C controller will have only
> one registers space.
> - Add "mscc,ocelot-i2c" example to test the previous fix.
> - Declare "unevaluatedProperties" property instead of
> "additionalProperties" one.
> - Due to the previous fix we can now discard the dummy boolean properties
> definitions, since the proper type evaluation will be performed by the
> generic i2c-controller.yaml schema.
> ---
> .../bindings/i2c/i2c-designware.txt | 73 ---------
> .../bindings/i2c/snps,designware-i2c.yaml | 154 ++++++++++++++++++
> 2 files changed, 154 insertions(+), 73 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware.txt
> create mode 100644 Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> deleted file mode 100644
> index 08be4d3846e5..000000000000
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ /dev/null
> @@ -1,73 +0,0 @@
> -* Synopsys DesignWare I2C
> -
> -Required properties :
> -
> - - compatible : should be "snps,designware-i2c"
> - or "mscc,ocelot-i2c" with "snps,designware-i2c" for fallback
> - - reg : Offset and length of the register set for the device
> - - interrupts : <IRQ> where IRQ is the interrupt number.
> - - clocks : phandles for the clocks, see the description of clock-names below.
> - The phandle for the "ic_clk" clock is required. The phandle for the "pclk"
> - clock is optional. If a single clock is specified but no clock-name, it is
> - the "ic_clk" clock. If both clocks are listed, the "ic_clk" must be first.
> -
> -Recommended properties :
> -
> - - clock-frequency : desired I2C bus clock frequency in Hz.
> -
> -Optional properties :
> -
> - - clock-names : Contains the names of the clocks:
> - "ic_clk", for the core clock used to generate the external I2C clock.
> - "pclk", the interface clock, required for register access.
> -
> - - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold
> - time, named ICPU_CFG:TWI_DELAY in the datasheet.
> -
> - - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
> - This option is only supported in hardware blocks version 1.11a or newer and
> - on Microsemi SoCs ("mscc,ocelot-i2c" compatible).
> -
> - - i2c-scl-falling-time-ns : should contain the SCL falling time in nanoseconds.
> - This value which is by default 300ns is used to compute the tLOW period.
> -
> - - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
> - This value which is by default 300ns is used to compute the tHIGH period.
> -
> -Examples :
> -
> - i2c@f0000 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - compatible = "snps,designware-i2c";
> - reg = <0xf0000 0x1000>;
> - interrupts = <11>;
> - clock-frequency = <400000>;
> - };
> -
> - i2c@1120000 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - compatible = "snps,designware-i2c";
> - reg = <0x1120000 0x1000>;
> - interrupt-parent = <&ictl>;
> - interrupts = <12 1>;
> - clock-frequency = <400000>;
> - i2c-sda-hold-time-ns = <300>;
> - i2c-sda-falling-time-ns = <300>;
> - i2c-scl-falling-time-ns = <300>;
> - };
> -
> - i2c@1120000 {
> - #address-cells = <1>;
> - #size-cells = <0>;
> - reg = <0x2000 0x100>;
> - clock-frequency = <400000>;
> - clocks = <&i2cclk>;
> - interrupts = <0>;
> -
> - eeprom@64 {
> - compatible = "linux,slave-24c02";
> - reg = <0x40000064>;
> - };
> - };
> diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> new file mode 100644
> index 000000000000..8d4e5fccbd1c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> @@ -0,0 +1,154 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/snps,designware-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys DesignWare APB I2C Controller
> +
> +maintainers:
> + - Jarkko Nikula <[email protected]>
> +
> +allOf:
> + - $ref: /schemas/i2c/i2c-controller.yaml#
> + - if:
> + properties:
> + compatible:
> + not:
> + contains:
> + const: mscc,ocelot-i2c
> + then:
> + properties:
> + reg:
> + maxItems: 1
> +
> +properties:
> + compatible:
> + oneOf:
> + - description: Generic Synopsys DesignWare I2C controller
> + const: snps,designware-i2c
> + - description: Microsemi Ocelot SoCs I2C controller
> + items:
> + - const: mscc,ocelot-i2c
> + - const: snps,designware-i2c
> +
> + reg:
> + minItems: 1
> + items:
> + - description: DW APB I2C controller memory mapped registers
> + - description: |
> + ICPU_CFG:TWI_DELAY registers to setup the SDA hold time.
> + This registers are specific to the Ocelot I2C-controller.
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + minItems: 1
> + items:
> + - description: I2C controller reference clock source
> + - description: APB interface clock source
> +
> + clock-names:
> + minItems: 1
> + items:
> + - const: ref
> + - const: pclk
> +
> + resets:
> + maxItems: 1
> +
> + clock-frequency:
> + description: Desired I2C bus clock frequency in Hz
> + enum: [100000, 400000, 1000000, 3400000]
> + default: 400000
> +
> + i2c-sda-hold-time-ns:
> + $ref: /schemas/types.yaml#/definitions/uint32
Don't need a type ref as properties with a unit-suffix already have one.
> + description: |
> + The property should contain the SDA hold time in nanoseconds. This option
> + is only supported in hardware blocks version 1.11a or newer or on
> + Microsemi SoCs.
> +
> + i2c-scl-falling-time-ns:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + The property should contain the SCL falling time in nanoseconds.
> + This value is used to compute the tLOW period.
> + default: 300
> +
> + i2c-sda-falling-time-ns:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: |
> + The property should contain the SDA falling time in nanoseconds.
> + This value is used to compute the tHIGH period.
> + default: 300
> +
> + dmas:
> + items:
> + - description: TX DMA Channel
> + - description: RX DMA Channel
> +
> + dma-names:
> + items:
> + - const: tx
> + - const: rx
> +
> +unevaluatedProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> + - interrupts
> +
> +examples:
> + - |
> + i2c@f0000 {
> + compatible = "snps,designware-i2c";
> + reg = <0xf0000 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <11>;
> + clock-frequency = <400000>;
> + };
> + - |
> + i2c@1120000 {
> + compatible = "snps,designware-i2c";
> + reg = <0x1120000 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <12 1>;
> + clock-frequency = <400000>;
> + i2c-sda-hold-time-ns = <300>;
> + i2c-sda-falling-time-ns = <300>;
> + i2c-scl-falling-time-ns = <300>;
> + };
> + - |
> + i2c@2000 {
> + compatible = "snps,designware-i2c";
> + reg = <0x2000 0x100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <400000>;
> + clocks = <&i2cclk>;
> + interrupts = <0>;
> +
> + eeprom@64 {
> + compatible = "linux,slave-24c02";
> + reg = <0x40000064>;
This causes 'make dt_binding_check' to fail. The unit-address should be
'40000064'. However, there's a bug in dtc not liking the high bits set
either. There's a fix pending, but I'd just fix the example here to
avoid the issue.
> + };
> + };
> + - |
> + i2c@100400 {
> + compatible = "mscc,ocelot-i2c", "snps,designware-i2c";
> + reg = <0x100400 0x100>, <0x198 0x8>;
> + pinctrl-0 = <&i2c_pins>;
> + pinctrl-names = "default";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <8>;
> + clocks = <&ahb_clk>;
> + };
> +...
> --
> 2.25.1
>
On Mon, May 11, 2020 at 11:09:24AM -0500, Rob Herring wrote:
> On Sun, May 10, 2020 at 12:50:08PM +0300, Serge Semin wrote:
> > Modern device tree bindings are supposed to be created as YAML-files
> > in accordance with dt-schema. This commit replaces Synopsys DW I2C
> > legacy bare text bindings with YAML file. As before the bindings file
> > states that the corresponding dts node is supposed to be compatible
> > either with generic DW I2C controller or with Microsemi Ocelot SoC I2C
> > one, to have registers, interrupts and clocks properties. In addition
> > the node may have clock-frequency, i2c-sda-hold-time-ns,
> > i2c-scl-falling-time-ns and i2c-sda-falling-time-ns optional properties.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Cc: Alexey Malahov <[email protected]>
> > Cc: Thomas Bogendoerfer <[email protected]>
> > Cc: Paul Burton <[email protected]>
> > Cc: Ralf Baechle <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Mika Westerberg <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Frank Rowand <[email protected]>
> > Cc: [email protected]
> >
> > ---
> >
> > Changelog v2:
> > - Make sure that "mscc,ocelot-i2c" compatible node may have up to two
> > registers space defined, while normal DW I2C controller will have only
> > one registers space.
> > - Add "mscc,ocelot-i2c" example to test the previous fix.
> > - Declare "unevaluatedProperties" property instead of
> > "additionalProperties" one.
> > - Due to the previous fix we can now discard the dummy boolean properties
> > definitions, since the proper type evaluation will be performed by the
> > generic i2c-controller.yaml schema.
> > ---
> > .../bindings/i2c/i2c-designware.txt | 73 ---------
> > .../bindings/i2c/snps,designware-i2c.yaml | 154 ++++++++++++++++++
> > 2 files changed, 154 insertions(+), 73 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > create mode 100644 Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > deleted file mode 100644
> > index 08be4d3846e5..000000000000
> > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > +++ /dev/null
> > @@ -1,73 +0,0 @@
> > -* Synopsys DesignWare I2C
> > -
> > -Required properties :
> > -
> > - - compatible : should be "snps,designware-i2c"
> > - or "mscc,ocelot-i2c" with "snps,designware-i2c" for fallback
> > - - reg : Offset and length of the register set for the device
> > - - interrupts : <IRQ> where IRQ is the interrupt number.
> > - - clocks : phandles for the clocks, see the description of clock-names below.
> > - The phandle for the "ic_clk" clock is required. The phandle for the "pclk"
> > - clock is optional. If a single clock is specified but no clock-name, it is
> > - the "ic_clk" clock. If both clocks are listed, the "ic_clk" must be first.
> > -
> > -Recommended properties :
> > -
> > - - clock-frequency : desired I2C bus clock frequency in Hz.
> > -
> > -Optional properties :
> > -
> > - - clock-names : Contains the names of the clocks:
> > - "ic_clk", for the core clock used to generate the external I2C clock.
> > - "pclk", the interface clock, required for register access.
> > -
> > - - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold
> > - time, named ICPU_CFG:TWI_DELAY in the datasheet.
> > -
> > - - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
> > - This option is only supported in hardware blocks version 1.11a or newer and
> > - on Microsemi SoCs ("mscc,ocelot-i2c" compatible).
> > -
> > - - i2c-scl-falling-time-ns : should contain the SCL falling time in nanoseconds.
> > - This value which is by default 300ns is used to compute the tLOW period.
> > -
> > - - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
> > - This value which is by default 300ns is used to compute the tHIGH period.
> > -
> > -Examples :
> > -
> > - i2c@f0000 {
> > - #address-cells = <1>;
> > - #size-cells = <0>;
> > - compatible = "snps,designware-i2c";
> > - reg = <0xf0000 0x1000>;
> > - interrupts = <11>;
> > - clock-frequency = <400000>;
> > - };
> > -
> > - i2c@1120000 {
> > - #address-cells = <1>;
> > - #size-cells = <0>;
> > - compatible = "snps,designware-i2c";
> > - reg = <0x1120000 0x1000>;
> > - interrupt-parent = <&ictl>;
> > - interrupts = <12 1>;
> > - clock-frequency = <400000>;
> > - i2c-sda-hold-time-ns = <300>;
> > - i2c-sda-falling-time-ns = <300>;
> > - i2c-scl-falling-time-ns = <300>;
> > - };
> > -
> > - i2c@1120000 {
> > - #address-cells = <1>;
> > - #size-cells = <0>;
> > - reg = <0x2000 0x100>;
> > - clock-frequency = <400000>;
> > - clocks = <&i2cclk>;
> > - interrupts = <0>;
> > -
> > - eeprom@64 {
> > - compatible = "linux,slave-24c02";
> > - reg = <0x40000064>;
> > - };
> > - };
> > diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > new file mode 100644
> > index 000000000000..8d4e5fccbd1c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > @@ -0,0 +1,154 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/i2c/snps,designware-i2c.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Synopsys DesignWare APB I2C Controller
> > +
> > +maintainers:
> > + - Jarkko Nikula <[email protected]>
> > +
> > +allOf:
> > + - $ref: /schemas/i2c/i2c-controller.yaml#
> > + - if:
> > + properties:
> > + compatible:
> > + not:
> > + contains:
> > + const: mscc,ocelot-i2c
> > + then:
> > + properties:
> > + reg:
> > + maxItems: 1
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - description: Generic Synopsys DesignWare I2C controller
> > + const: snps,designware-i2c
> > + - description: Microsemi Ocelot SoCs I2C controller
> > + items:
> > + - const: mscc,ocelot-i2c
> > + - const: snps,designware-i2c
> > +
> > + reg:
> > + minItems: 1
> > + items:
> > + - description: DW APB I2C controller memory mapped registers
> > + - description: |
> > + ICPU_CFG:TWI_DELAY registers to setup the SDA hold time.
> > + This registers are specific to the Ocelot I2C-controller.
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + minItems: 1
> > + items:
> > + - description: I2C controller reference clock source
> > + - description: APB interface clock source
> > +
> > + clock-names:
> > + minItems: 1
> > + items:
> > + - const: ref
> > + - const: pclk
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + clock-frequency:
> > + description: Desired I2C bus clock frequency in Hz
> > + enum: [100000, 400000, 1000000, 3400000]
> > + default: 400000
> > +
> > + i2c-sda-hold-time-ns:
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> Don't need a type ref as properties with a unit-suffix already have one.
Ok. Seeing "-ns" suffixed properties have uint32-array type I'll have to add the
restriction "maxItems: 1" here then.
>
> > + description: |
> > + The property should contain the SDA hold time in nanoseconds. This option
> > + is only supported in hardware blocks version 1.11a or newer or on
> > + Microsemi SoCs.
> > +
> > + i2c-scl-falling-time-ns:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + The property should contain the SCL falling time in nanoseconds.
> > + This value is used to compute the tLOW period.
> > + default: 300
> > +
> > + i2c-sda-falling-time-ns:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description: |
> > + The property should contain the SDA falling time in nanoseconds.
> > + This value is used to compute the tHIGH period.
> > + default: 300
> > +
> > + dmas:
> > + items:
> > + - description: TX DMA Channel
> > + - description: RX DMA Channel
> > +
> > + dma-names:
> > + items:
> > + - const: tx
> > + - const: rx
> > +
> > +unevaluatedProperties: false
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - "#address-cells"
> > + - "#size-cells"
> > + - interrupts
> > +
> > +examples:
> > + - |
> > + i2c@f0000 {
> > + compatible = "snps,designware-i2c";
> > + reg = <0xf0000 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + interrupts = <11>;
> > + clock-frequency = <400000>;
> > + };
> > + - |
> > + i2c@1120000 {
> > + compatible = "snps,designware-i2c";
> > + reg = <0x1120000 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + interrupts = <12 1>;
> > + clock-frequency = <400000>;
> > + i2c-sda-hold-time-ns = <300>;
> > + i2c-sda-falling-time-ns = <300>;
> > + i2c-scl-falling-time-ns = <300>;
> > + };
> > + - |
> > + i2c@2000 {
> > + compatible = "snps,designware-i2c";
> > + reg = <0x2000 0x100>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + clock-frequency = <400000>;
> > + clocks = <&i2cclk>;
> > + interrupts = <0>;
> > +
> > + eeprom@64 {
> > + compatible = "linux,slave-24c02";
> > + reg = <0x40000064>;
>
> This causes 'make dt_binding_check' to fail. The unit-address should be
> '40000064'. However, there's a bug in dtc not liking the high bits set
> either. There's a fix pending, but I'd just fix the example here to
> avoid the issue.
Do you mean a fix suggested in this patchset or someplace else? I've submitted
it before this patch so to solve the problem you've discovered.
Regarding the example. Do you suggest to completely remove the eeprom
sub-node? I am asking because without proper reg flags setting the i2c-slave
mode example would be just wrong.
-Sergey
>
> > + };
> > + };
> > + - |
> > + i2c@100400 {
> > + compatible = "mscc,ocelot-i2c", "snps,designware-i2c";
> > + reg = <0x100400 0x100>, <0x198 0x8>;
> > + pinctrl-0 = <&i2c_pins>;
> > + pinctrl-names = "default";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + interrupts = <8>;
> > + clocks = <&ahb_clk>;
> > + };
> > +...
> > --
> > 2.25.1
> >
On Sun, May 10, 2020 at 12:50:09PM +0300, Serge Semin wrote:
> Add the "baikal,bt1-sys-i2c" compatible string to the DW I2C binding and
> make sure the reg property isn't required in this case because the
> controller is embedded into the Baikal-T1 System Controller. The rest of
> the DW APB I2C properties are compatible and can be freely used to describe
> the Baikal-T1 I2C controller dts-node.
Is there not a sub-range of the system controller with the I2C
registers? I'd assume there is, so you can still have 'reg' even if
Linux doesn't use it (currently).
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Mika Westerberg <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
>
> ---
>
> Rob, I had to remove your acked-by tag because of the changes introduced
> in v2 of the patch.
>
> Changelog v2:
> - Make the reg property being optional if it's Baikal-T1 System I2C DT
> node.
> ---
> .../devicetree/bindings/i2c/snps,designware-i2c.yaml | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> index 8d4e5fccbd1c..579964098eb9 100644
> --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> @@ -21,6 +21,15 @@ allOf:
> properties:
> reg:
> maxItems: 1
> + - if:
> + properties:
> + compatible:
> + not:
> + contains:
> + const: baikal,bt1-sys-i2c
> + then:
> + required:
> + - reg
>
> properties:
> compatible:
> @@ -31,6 +40,8 @@ properties:
> items:
> - const: mscc,ocelot-i2c
> - const: snps,designware-i2c
> + - description: Baikal-T1 SoC System I2C controller
> + const: baikal,bt1-sys-i2c
>
> reg:
> minItems: 1
> @@ -98,7 +109,6 @@ unevaluatedProperties: false
>
> required:
> - compatible
> - - reg
> - "#address-cells"
> - "#size-cells"
> - interrupts
> --
> 2.25.1
>
On Mon, May 18, 2020 at 02:33:19PM -0600, Rob Herring wrote:
> On Sun, May 10, 2020 at 12:50:09PM +0300, Serge Semin wrote:
> > Add the "baikal,bt1-sys-i2c" compatible string to the DW I2C binding and
> > make sure the reg property isn't required in this case because the
> > controller is embedded into the Baikal-T1 System Controller. The rest of
> > the DW APB I2C properties are compatible and can be freely used to describe
> > the Baikal-T1 I2C controller dts-node.
>
> Is there not a sub-range of the system controller with the I2C
> registers? I'd assume there is, so you can still have 'reg' even if
> Linux doesn't use it (currently).
Yes, there is a range. It's just three access registers. Is it wrong to make the
reg property being optional in this case since it can be accessed over syscon
regmap? Do you suggest to get back the reg property being required for our
device?
-Sergey
>
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Cc: Alexey Malahov <[email protected]>
> > Cc: Thomas Bogendoerfer <[email protected]>
> > Cc: Paul Burton <[email protected]>
> > Cc: Ralf Baechle <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Mika Westerberg <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Frank Rowand <[email protected]>
> > Cc: [email protected]
> >
> > ---
> >
> > Rob, I had to remove your acked-by tag because of the changes introduced
> > in v2 of the patch.
> >
> > Changelog v2:
> > - Make the reg property being optional if it's Baikal-T1 System I2C DT
> > node.
> > ---
> > .../devicetree/bindings/i2c/snps,designware-i2c.yaml | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > index 8d4e5fccbd1c..579964098eb9 100644
> > --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> > @@ -21,6 +21,15 @@ allOf:
> > properties:
> > reg:
> > maxItems: 1
> > + - if:
> > + properties:
> > + compatible:
> > + not:
> > + contains:
> > + const: baikal,bt1-sys-i2c
> > + then:
> > + required:
> > + - reg
> >
> > properties:
> > compatible:
> > @@ -31,6 +40,8 @@ properties:
> > items:
> > - const: mscc,ocelot-i2c
> > - const: snps,designware-i2c
> > + - description: Baikal-T1 SoC System I2C controller
> > + const: baikal,bt1-sys-i2c
> >
> > reg:
> > minItems: 1
> > @@ -98,7 +109,6 @@ unevaluatedProperties: false
> >
> > required:
> > - compatible
> > - - reg
> > - "#address-cells"
> > - "#size-cells"
> > - interrupts
> > --
> > 2.25.1
> >
On 5/10/20 12:50 PM, Serge Semin wrote:
> DW APB I2C slave code in fact depends on the DW I2C driver core, but not
> on the platform code. Yes, the I2C slave interface is currently supported
> by the platform version of the IP core, but it doesn't make it dependent
> on it. So make sure the DW APB I2C slave config is only available if the
> I2C_DESIGNWARE_CORE config is enabled.
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Mika Westerberg <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Jarkko Nikula <[email protected]>
On 5/10/20 12:50 PM, Serge Semin wrote:
> Since commit 4f8272802739 ("Documentation: update kbuild loadable modules
> goals & examples") `-objs` is fitted for building host programs, lets
> change DW I2C core, platform and PCI driver kbuild directives to using
> `-y`, which more straightforward for device drivers. By doing so we can
> discard the ifeq construction in favor to the more natural and less bulky
> `<module>-$(CONFIG_X) += x.o`
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Mika Westerberg <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/i2c/busses/Makefile | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
Acked-by: Jarkko Nikula <[email protected]>
On 5/10/20 12:50 PM, Serge Semin wrote:
> Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
> platform driver. It's a bit confusing to see it's config in the menu at
> some separated place with no reference to the platform code. Lets move the
> config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
> the config menu will display the feature right below the DW I2C platform
> driver item and will indent it to the right so signifying its belonging.
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Mika Westerberg <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 368aa64e9266..ed6927c4c540 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
>
> config I2C_DESIGNWARE_PLATFORM
> tristate "Synopsys DesignWare Platform"
> - select I2C_DESIGNWARE_CORE
> depends on (ACPI && COMMON_CLK) || !ACPI
> + select I2C_DESIGNWARE_CORE
> help
> If you say yes to this option, support will be included for the
> Synopsys DesignWare I2C adapter.
> @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
> This driver can also be built as a module. If so, the module
> will be called i2c-designware-platform.
>
> +if I2C_DESIGNWARE_PLATFORM
> +
> +config I2C_DESIGNWARE_BAYTRAIL
> + bool "Intel Baytrail I2C semaphore support"
> + depends on ACPI
> + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
> + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
> + help
> + This driver enables managed host access to the PMIC I2C bus on select
> + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
> + the host to request uninterrupted access to the PMIC's I2C bus from
> + the platform firmware controlling it. You should say Y if running on
> + a BayTrail system using the AXP288.
> +
> +endif # I2C_DESIGNWARE_PLATFORM
> +
Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the
"depends on" be enough?
Jarkko
On 5/10/20 12:50 PM, Serge Semin wrote:
> Seeing the DW I2C driver is using flags-based accessors with two
> conditional clauses it would be better to replace them with the regmap
> API IO methods and to initialize the regmap object with read/write
> callbacks specific to the controller registers map implementation. This
> will be also handy for the drivers with non-standard registers mapping
> (like an embedded into the Baikal-T1 System Controller DW I2C block, which
> glue-driver is a part of this series).
>
> As before the driver tries to detect the mapping setup at probe stage and
> creates a regmap object accordingly, which will be used by the rest of the
> code to correctly access the controller registers. In two places it was
> appropriate to convert the hand-written read-modify-write and
> read-poll-loop design patterns to the corresponding regmap API
> ready-to-use methods.
>
> Note the regmap IO methods return value is checked only at the probe
> stage. The rest of the code won't do this because basically we have
> MMIO-based regmap so non of the read/write methods can fail (this also
> won't be needed for the Baikal-T1-specific I2C controller).
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Wolfram Sang <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/i2c/busses/Kconfig | 1 +
> drivers/i2c/busses/i2c-designware-common.c | 171 +++++++++++++++------
> drivers/i2c/busses/i2c-designware-core.h | 18 +--
> drivers/i2c/busses/i2c-designware-master.c | 125 ++++++++-------
> drivers/i2c/busses/i2c-designware-slave.c | 77 +++++-----
> 5 files changed, 239 insertions(+), 153 deletions(-)
>
Looking at patches 4/12-12/12 I think it would be good to move fixes and
less invasive patches before this. Like
i2c: designware: slave: Set DW I2C core module dependency
i2c: designware: Use `-y` to build multi-object modules
i2c: designware: Move Baytrail sem config to the platform if-clause
That said, you may add:
Tested-by: Jarkko Nikula <[email protected]>
Acked-by: Jarkko Nikula <[email protected]>
On 5/10/20 12:50 PM, Serge Semin wrote:
> A PM workaround activated by the flag MODEL_CHERRYTRAIL has been removed
> since commit 9cbeeca05049 ("i2c: designware: Remove Cherry Trail PMIC I2C
> bus pm_disabled workaround"), but the flag most likely by mistake has been
> left in the Dw I2C drivers. Lets remove it.
>
> By doing so we get rid from the last DW APB I2C IP-core model flag, so we
> can remove the MODEL_MASK macro too.
>
> Signed-off-by: Serge Semin <[email protected]>
> Cc: Alexey Malahov <[email protected]>
> Cc: Thomas Bogendoerfer <[email protected]>
> Cc: Paul Burton <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/i2c/busses/i2c-designware-core.h | 3 ---
> drivers/i2c/busses/i2c-designware-pcidrv.c | 1 -
> drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
> 3 files changed, 1 insertion(+), 5 deletions(-)
>
Acked-by: Jarkko Nikula <[email protected]>
Hi
On 5/10/20 12:50 PM, Serge Semin wrote:
> Seeing the DW I2C platform driver is getting overcomplicated with a lot of
> vendor-specific configs let's introduce a glue-layer interface so new
> platforms which equipped with Synopsys Designware APB I2C IP-core would
> be able to handle their peculiarities in the dedicated objects.
>
Comment to this patch and patches 9/12 and 12/12:
Currently i2c-designware-platdrv.c is about 500 lines of code so I don't
think it's too overcomplicated. But I feel we have already too many
Kconfig options and source modules for i2c-designware and obviously
would like to push back a little from adding more.
I don't think i2c-designware-platdrv.c becomes yet too complicated if
Baikal related code is added there, perhaps under #ifdef CONFIG_OF like
MSCC Ocelot code is currently.
--
Jarkko
On Wed, May 20, 2020 at 03:16:07PM +0300, Jarkko Nikula wrote:
> On 5/10/20 12:50 PM, Serge Semin wrote:
> > Seeing the DW I2C driver is using flags-based accessors with two
> > conditional clauses it would be better to replace them with the regmap
> > API IO methods and to initialize the regmap object with read/write
> > callbacks specific to the controller registers map implementation. This
> > will be also handy for the drivers with non-standard registers mapping
> > (like an embedded into the Baikal-T1 System Controller DW I2C block, which
> > glue-driver is a part of this series).
> >
> > As before the driver tries to detect the mapping setup at probe stage and
> > creates a regmap object accordingly, which will be used by the rest of the
> > code to correctly access the controller registers. In two places it was
> > appropriate to convert the hand-written read-modify-write and
> > read-poll-loop design patterns to the corresponding regmap API
> > ready-to-use methods.
> >
> > Note the regmap IO methods return value is checked only at the probe
> > stage. The rest of the code won't do this because basically we have
> > MMIO-based regmap so non of the read/write methods can fail (this also
> > won't be needed for the Baikal-T1-specific I2C controller).
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Serge Semin <[email protected]>
> > Cc: Alexey Malahov <[email protected]>
> > Cc: Thomas Bogendoerfer <[email protected]>
> > Cc: Paul Burton <[email protected]>
> > Cc: Ralf Baechle <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Frank Rowand <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/i2c/busses/Kconfig | 1 +
> > drivers/i2c/busses/i2c-designware-common.c | 171 +++++++++++++++------
> > drivers/i2c/busses/i2c-designware-core.h | 18 +--
> > drivers/i2c/busses/i2c-designware-master.c | 125 ++++++++-------
> > drivers/i2c/busses/i2c-designware-slave.c | 77 +++++-----
> > 5 files changed, 239 insertions(+), 153 deletions(-)
> >
> Looking at patches 4/12-12/12 I think it would be good to move fixes and
> less invasive patches before this. Like
>
> i2c: designware: slave: Set DW I2C core module dependency
> i2c: designware: Use `-y` to build multi-object modules
> i2c: designware: Move Baytrail sem config to the platform if-clause
>
> That said, you may add:
>
> Tested-by: Jarkko Nikula <[email protected]>
> Acked-by: Jarkko Nikula <[email protected]>
Ok. I'll move those three patches to be before this one in v3. Thanks.
-Sergey
On Wed, May 20, 2020 at 03:16:14PM +0300, Jarkko Nikula wrote:
> On 5/10/20 12:50 PM, Serge Semin wrote:
> > Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
> > platform driver. It's a bit confusing to see it's config in the menu at
> > some separated place with no reference to the platform code. Lets move the
> > config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
> > the config menu will display the feature right below the DW I2C platform
> > driver item and will indent it to the right so signifying its belonging.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Cc: Alexey Malahov <[email protected]>
> > Cc: Thomas Bogendoerfer <[email protected]>
> > Cc: Paul Burton <[email protected]>
> > Cc: Ralf Baechle <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Mika Westerberg <[email protected]>
> > Cc: Wolfram Sang <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Frank Rowand <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
> > 1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 368aa64e9266..ed6927c4c540 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
> > config I2C_DESIGNWARE_PLATFORM
> > tristate "Synopsys DesignWare Platform"
> > - select I2C_DESIGNWARE_CORE
> > depends on (ACPI && COMMON_CLK) || !ACPI
> > + select I2C_DESIGNWARE_CORE
> > help
> > If you say yes to this option, support will be included for the
> > Synopsys DesignWare I2C adapter.
> > @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
> > This driver can also be built as a module. If so, the module
> > will be called i2c-designware-platform.
> > +if I2C_DESIGNWARE_PLATFORM
> > +
> > +config I2C_DESIGNWARE_BAYTRAIL
> > + bool "Intel Baytrail I2C semaphore support"
> > + depends on ACPI
> > + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
> > + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
> > + help
> > + This driver enables managed host access to the PMIC I2C bus on select
> > + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
> > + the host to request uninterrupted access to the PMIC's I2C bus from
> > + the platform firmware controlling it. You should say Y if running on
> > + a BayTrail system using the AXP288.
> > +
> > +endif # I2C_DESIGNWARE_PLATFORM
> > +
>
> Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends
> on" be enough?
The idea was to add if-endif clause here for features possibly added sometime
in future. But using normal "depends on I2C_DESIGNWARE_PLATFORM" shall make
the config depicted as an indented sub-config as well. Would you like me to
remove the if-clause and use the depends on operator instead?
-Sergey
>
> Jarkko
On Wed, May 20, 2020 at 03:46:11PM +0300, Jarkko Nikula wrote:
> Hi
>
> On 5/10/20 12:50 PM, Serge Semin wrote:
> > Seeing the DW I2C platform driver is getting overcomplicated with a lot of
> > vendor-specific configs let's introduce a glue-layer interface so new
> > platforms which equipped with Synopsys Designware APB I2C IP-core would
> > be able to handle their peculiarities in the dedicated objects.
> >
> Comment to this patch and patches 9/12 and 12/12:
>
> Currently i2c-designware-platdrv.c is about 500 lines of code so I don't
> think it's too overcomplicated. But I feel we have already too many Kconfig
> options and source modules for i2c-designware and obviously would like to
> push back a little from adding more.
>
> I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal
> related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot
> code is currently.
Well, it's up to you to decide, what solution is more suitable for you to
maintain. My idea of detaching the MSCC and Baikal-T1 code to the dedicated
source files was to eventually move the whole i2c-designware-* set of files
into a dedicated directory drivers/i2c/buses/dw as it's done for some others
Synopsys DesignWare controllers: drivers/pci/controller/dwc/, drivers/usb/dwc2,
drivers/usb/dwc3, drivers/net/ethernet/synopsys/ . If you think, that it's too
early for Dw I2C code to live in a dedicated directory, fine with me. I can
merge the MSCC and Baikal-T1 code back into the i2c-designware-platdrv.c .
So what's your final word in this matter?
-Sergey
>
> --
> Jarkko
On 5/21/20 5:22 AM, Serge Semin wrote:
> On Wed, May 20, 2020 at 03:16:14PM +0300, Jarkko Nikula wrote:
>> On 5/10/20 12:50 PM, Serge Semin wrote:
>>> Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
>>> platform driver. It's a bit confusing to see it's config in the menu at
>>> some separated place with no reference to the platform code. Lets move the
>>> config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
>>> the config menu will display the feature right below the DW I2C platform
>>> driver item and will indent it to the right so signifying its belonging.
>>>
>>> Signed-off-by: Serge Semin <[email protected]>
>>> Cc: Alexey Malahov <[email protected]>
>>> Cc: Thomas Bogendoerfer <[email protected]>
>>> Cc: Paul Burton <[email protected]>
>>> Cc: Ralf Baechle <[email protected]>
>>> Cc: Andy Shevchenko <[email protected]>
>>> Cc: Mika Westerberg <[email protected]>
>>> Cc: Wolfram Sang <[email protected]>
>>> Cc: Rob Herring <[email protected]>
>>> Cc: Frank Rowand <[email protected]>
>>> Cc: [email protected]
>>> Cc: [email protected]
>>> ---
>>> drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
>>> 1 file changed, 17 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>> index 368aa64e9266..ed6927c4c540 100644
>>> --- a/drivers/i2c/busses/Kconfig
>>> +++ b/drivers/i2c/busses/Kconfig
>>> @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
>>> config I2C_DESIGNWARE_PLATFORM
>>> tristate "Synopsys DesignWare Platform"
>>> - select I2C_DESIGNWARE_CORE
>>> depends on (ACPI && COMMON_CLK) || !ACPI
>>> + select I2C_DESIGNWARE_CORE
>>> help
>>> If you say yes to this option, support will be included for the
>>> Synopsys DesignWare I2C adapter.
>>> @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
>>> This driver can also be built as a module. If so, the module
>>> will be called i2c-designware-platform.
>>> +if I2C_DESIGNWARE_PLATFORM
>>> +
>>> +config I2C_DESIGNWARE_BAYTRAIL
>>> + bool "Intel Baytrail I2C semaphore support"
>>> + depends on ACPI
>>> + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
>>> + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
>>> + help
>>> + This driver enables managed host access to the PMIC I2C bus on select
>>> + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
>>> + the host to request uninterrupted access to the PMIC's I2C bus from
>>> + the platform firmware controlling it. You should say Y if running on
>>> + a BayTrail system using the AXP288.
>>> +
>>> +endif # I2C_DESIGNWARE_PLATFORM
>>> +
>>
>> Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends
>> on" be enough?
>
> The idea was to add if-endif clause here for features possibly added sometime
> in future. But using normal "depends on I2C_DESIGNWARE_PLATFORM" shall make
> the config depicted as an indented sub-config as well. Would you like me to
> remove the if-clause and use the depends on operator instead?
>
Yes, please remove it from this patch. Keeps this patch simpler and if
some future feature needs it then that patch(set) is the right place to
add it.
Jarkko
Hi
On 5/21/20 5:37 AM, Serge Semin wrote:
> On Wed, May 20, 2020 at 03:46:11PM +0300, Jarkko Nikula wrote:
>> Hi
>>
>> On 5/10/20 12:50 PM, Serge Semin wrote:
>>> Seeing the DW I2C platform driver is getting overcomplicated with a lot of
>>> vendor-specific configs let's introduce a glue-layer interface so new
>>> platforms which equipped with Synopsys Designware APB I2C IP-core would
>>> be able to handle their peculiarities in the dedicated objects.
>>>
>> Comment to this patch and patches 9/12 and 12/12:
>>
>> Currently i2c-designware-platdrv.c is about 500 lines of code so I don't
>> think it's too overcomplicated. But I feel we have already too many Kconfig
>> options and source modules for i2c-designware and obviously would like to
>> push back a little from adding more.
>>
>> I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal
>> related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot
>> code is currently.
>
> Well, it's up to you to decide, what solution is more suitable for you to
> maintain. My idea of detaching the MSCC and Baikal-T1 code to the dedicated
> source files was to eventually move the whole i2c-designware-* set of files
> into a dedicated directory drivers/i2c/buses/dw as it's done for some others
> Synopsys DesignWare controllers: drivers/pci/controller/dwc/, drivers/usb/dwc2,
> drivers/usb/dwc3, drivers/net/ethernet/synopsys/ . If you think, that it's too
> early for Dw I2C code to live in a dedicated directory, fine with me. I can
> merge the MSCC and Baikal-T1 code back into the i2c-designware-platdrv.c .
> So what's your final word in this matter?
>
I think sub directory decision under each subsystem is more subsystem
rather than vendor/driver specific. Good point anyway.
For this patchset I'd like more if changes are done to
i2c-designware-platdrv.c since it's not too complicated yet :-)
If it starts to look too messy in the future then it's time split I think.
Jarkko
On Mon, May 25, 2020 at 04:16:05PM +0300, Jarkko Nikula wrote:
> On 5/21/20 5:37 AM, Serge Semin wrote:
> For this patchset I'd like more if changes are done to
> i2c-designware-platdrv.c since it's not too complicated yet :-)
And after moving ACPI stuff to common code, the one has even been shrunk significantly.
> If it starts to look too messy in the future then it's time split I think.
--
With Best Regards,
Andy Shevchenko
On Mon, May 25, 2020 at 04:01:26PM +0300, Jarkko Nikula wrote:
> On 5/21/20 5:22 AM, Serge Semin wrote:
> > On Wed, May 20, 2020 at 03:16:14PM +0300, Jarkko Nikula wrote:
> > > On 5/10/20 12:50 PM, Serge Semin wrote:
> > > > Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
> > > > platform driver. It's a bit confusing to see it's config in the menu at
> > > > some separated place with no reference to the platform code. Lets move the
> > > > config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
> > > > the config menu will display the feature right below the DW I2C platform
> > > > driver item and will indent it to the right so signifying its belonging.
> > > >
> > > > Signed-off-by: Serge Semin <[email protected]>
> > > > Cc: Alexey Malahov <[email protected]>
> > > > Cc: Thomas Bogendoerfer <[email protected]>
> > > > Cc: Paul Burton <[email protected]>
> > > > Cc: Ralf Baechle <[email protected]>
> > > > Cc: Andy Shevchenko <[email protected]>
> > > > Cc: Mika Westerberg <[email protected]>
> > > > Cc: Wolfram Sang <[email protected]>
> > > > Cc: Rob Herring <[email protected]>
> > > > Cc: Frank Rowand <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > ---
> > > > drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
> > > > 1 file changed, 17 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > > > index 368aa64e9266..ed6927c4c540 100644
> > > > --- a/drivers/i2c/busses/Kconfig
> > > > +++ b/drivers/i2c/busses/Kconfig
> > > > @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
> > > > config I2C_DESIGNWARE_PLATFORM
> > > > tristate "Synopsys DesignWare Platform"
> > > > - select I2C_DESIGNWARE_CORE
> > > > depends on (ACPI && COMMON_CLK) || !ACPI
> > > > + select I2C_DESIGNWARE_CORE
> > > > help
> > > > If you say yes to this option, support will be included for the
> > > > Synopsys DesignWare I2C adapter.
> > > > @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
> > > > This driver can also be built as a module. If so, the module
> > > > will be called i2c-designware-platform.
> > > > +if I2C_DESIGNWARE_PLATFORM
> > > > +
> > > > +config I2C_DESIGNWARE_BAYTRAIL
> > > > + bool "Intel Baytrail I2C semaphore support"
> > > > + depends on ACPI
> > > > + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
> > > > + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
> > > > + help
> > > > + This driver enables managed host access to the PMIC I2C bus on select
> > > > + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
> > > > + the host to request uninterrupted access to the PMIC's I2C bus from
> > > > + the platform firmware controlling it. You should say Y if running on
> > > > + a BayTrail system using the AXP288.
> > > > +
> > > > +endif # I2C_DESIGNWARE_PLATFORM
> > > > +
> > >
> > > Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends
> > > on" be enough?
> >
> > The idea was to add if-endif clause here for features possibly added sometime
> > in future. But using normal "depends on I2C_DESIGNWARE_PLATFORM" shall make
> > the config depicted as an indented sub-config as well. Would you like me to
> > remove the if-clause and use the depends on operator instead?
> >
> Yes, please remove it from this patch. Keeps this patch simpler and if some
> future feature needs it then that patch(set) is the right place to add it.
Agreed. I'll do this in v3.
-Sergey
>
> Jarkko
On Mon, May 25, 2020 at 04:16:05PM +0300, Jarkko Nikula wrote:
> Hi
>
> On 5/21/20 5:37 AM, Serge Semin wrote:
> > On Wed, May 20, 2020 at 03:46:11PM +0300, Jarkko Nikula wrote:
> > > Hi
> > >
> > > On 5/10/20 12:50 PM, Serge Semin wrote:
> > > > Seeing the DW I2C platform driver is getting overcomplicated with a lot of
> > > > vendor-specific configs let's introduce a glue-layer interface so new
> > > > platforms which equipped with Synopsys Designware APB I2C IP-core would
> > > > be able to handle their peculiarities in the dedicated objects.
> > > >
> > > Comment to this patch and patches 9/12 and 12/12:
> > >
> > > Currently i2c-designware-platdrv.c is about 500 lines of code so I don't
> > > think it's too overcomplicated. But I feel we have already too many Kconfig
> > > options and source modules for i2c-designware and obviously would like to
> > > push back a little from adding more.
> > >
> > > I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal
> > > related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot
> > > code is currently.
> >
> > Well, it's up to you to decide, what solution is more suitable for you to
> > maintain. My idea of detaching the MSCC and Baikal-T1 code to the dedicated
> > source files was to eventually move the whole i2c-designware-* set of files
> > into a dedicated directory drivers/i2c/buses/dw as it's done for some others
> > Synopsys DesignWare controllers: drivers/pci/controller/dwc/, drivers/usb/dwc2,
> > drivers/usb/dwc3, drivers/net/ethernet/synopsys/ . If you think, that it's too
> > early for Dw I2C code to live in a dedicated directory, fine with me. I can
> > merge the MSCC and Baikal-T1 code back into the i2c-designware-platdrv.c .
> > So what's your final word in this matter?
> >
> I think sub directory decision under each subsystem is more subsystem rather
> than vendor/driver specific. Good point anyway.
>
> For this patchset I'd like more if changes are done to
> i2c-designware-platdrv.c since it's not too complicated yet :-)
>
> If it starts to look too messy in the future then it's time split I think.
Ok. I'll merge the MSCC back and add Baikal-T1 System I2C support into the
DW I2C platform driver.
-Sergey
>
> Jarkko