Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On 03/01/2014 19:59, Jason Gunthorpe wrote: > On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote: > >> +/* SoC ID */ >> +soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16; >> + >> +/* SoC revision */ >> +soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF) >> +& SOC_REV_MASK; > > Sort of a minor nit, but I'm not actually sure these registers are > read-only :( I know the documentation doesn't describe how to change > them, but in PCI-E EndPoint mode they are read by the host so the > firmware must be able to change them to reflect the firmware running > on the endpoint.. According to the datasheet of the Armada XP and the Armada 370 these register are read-only. Moreover they are already use as is for the kirkwood, orion5x, dove and mv78x00 in the current kernel and for all the mvebu Socs in the internal version of Marvell, so even if it was possible to change these values I believe that it never happened. > > Which is to say, I suppose the bootloader could program them to > something else if it wanted to. > > That said, in practice obviously they will not be changed, but maybe > there is another ID register you can read? > > Jason > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
On 03/01/2014 19:49, Thomas Petazzoni wrote: > Dear Gregory CLEMENT, > > On Fri, 3 Jan 2014 10:59:45 +0100, Gregory CLEMENT wrote: > >> +/* >> + * Only revison more recent than A0 support offload > > revison -> revisions > > offload mechanism -> the offload mechanism > >> + * mechanism. In case we can't get the SoC revision >> + * weplay safe and we don't enable it > > weplay -> we play > >> + */ >> +if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV) >> +drv_data->offload_enabled = true; >> } >> >> out: > These typos will be fixed in the next version. > Thanks! > > Thomas > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On 03/01/2014 17:41, Andrew Lunn wrote:
Hi Gregory
I'm away from my hardware at the moment.
Does this work when all the PCIe ports have status = "disabled";? We
have many kirkwood devices in NAS boxes which don't use PCIe, so all
the ports are disabled. But they still exist in the SoC, so we can
read the IDs from them. I just don't know if of_get_next_child() will
only return enabled children?
>>>
>>> There is a function named of_get_next_available_child, so I assumed that
>>> of_get_next_child() will return all the children. But I can test it to
>>> be sure of it.
>>>
>>
>> I have just removed all the PCIe part in the
>> armada-xp-openblocks-ax3-4.dts file (PCie is disable by default in the
>> dtsi file) and it worled as expected! :)
>
> Great, thanks for testing.
>
>> by the way waht do you think of adding this line in at the end of the
>> mvebu_soc_id_init() function:
>>
>> pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
>
> Kirkwood prints actual strings, not numbers. More readable.
With this number we are sure to have always an information even if the
SoC ID or version was unknown by the kernel.
>
>> Also keep in mind that currently you can't use it for kirkwood because
>> the build of the file depend on CONFIG_ARCH_MVEBU. But as kirkwood
>> will soon joined the mach-mvebu directory, it won't be a problem then.
>
> I think it is possible to do ../mach-mvebu/soc_id.o sort of thing in
> the Makefile. Ugly, but might work until we move.
Yes it is doable, but then we also need to update the mvebu_pcie_of_match_table.
However I would prefer doing this as a separate patch because this one is
for fixing a bug. Later we can improve the kernel.
>
> Andrew
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On 03/01/2014 19:48, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
>
> On Fri, 3 Jan 2014 10:59:44 +0100, Gregory CLEMENT wrote:
>> +static int __init mvebu_soc_id_init(void)
>> +{
>> +struct device_node *np;
>> +int ret = 0;
>> +
>> +np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
>> +if (np) {
>
> Change this to:
>
> if (!np)
> return 0;
>
> so that you avoid indenting the entire function code inside the if
> { ... } block.
ok
>
>> +void __iomem *pci_base;
>> +struct clk *clk;
>> +/*
>> + * ID and revision are available from any port, so we
>> + * just pick the first one
>> + */
>> +struct device_node *child = of_get_next_child(np, NULL);
>
> Maybe check that child is not NULL here?
yes I was a little lazy with it: if child is NULL then of_clk_get_by_name
will return an error but then the error message will be a misleading.
>
>> +
>> +clk = of_clk_get_by_name(child, NULL);
>> +if (IS_ERR(clk)) {
>> +pr_err("%s: cannot get clock\n", __func__);
>
> I think you should rather do:
>
> #define pr_fmt(fmt) "mvebu-soc-id: " fmt
>
> at the beginning of your C file and get rid of the "%s" for the
> __func__.
ok thanks for the tip
>
>> +/* SoC ID */
>> +soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
>> +
>> +/* SoC revision */
>> +soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
>> +& SOC_REV_MASK;
>
> Use readl() for both of these reads. __raw_readl() will not work
> properly when the system is booted big-endian.
ok
>
>> +return ret;
>> +}
>> +arch_initcall(mvebu_soc_id_init);
>
>> +#ifdef CONFIG_ARCH_MVEBU
>> +int mvebu_get_soc_id(u32 *dev, u32 *rev);
>> +#else
>> +int mvebu_get_soc_id(u32 *dev, u32 *rev)
>
> Missing "static inline". Without these, if this header file is included
> more than once, you will have two times the same symbol defined.
>
ok
> Best regards,
>
> Thomas
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 REPOST 3/4] i2c: i2c-ibm-iic: Implements transfer abortion
2014/1/3 Wolfram Sang : > On Fri, Dec 20, 2013 at 04:12:55PM +0100, jean-jacques hiblot wrote: >> From: jean-jacques hiblot >> >> Clean-up properly when a transfer fails for whatever reason. >> Cancel the transfer when the process is signaled. > > Please describe what you do a little. I wonder how you can remove so > much code while keeping the functionality? Well there are 2 reasons why so much code went away. 1) The iic_wait_for_tc() function wasn't used anymore (it should have disappeared in an earlier patch but the diff was terrible to read) 2) the whole abortion scheme is different. It's now done as a part of the data transfer. The reason is that the controller doesn't react properly to abortion when it's not done at the right moment. The idea here is to abort the transfer right after sending the next byte to keep the controller happy. If the abortion is asked at the wrong moment, the controller may not set the abortion complete flag and the next transfer may fail. > >> >> Signed-off-by: jean-jacques hiblot > >> - out_8(&iic->cntl, CNTL_HMT); >> + DBG(dev, "aborting transfer\n"); >> + /* transfer should be aborted within 10ms */ >> + end = jiffies + 10; > > Eeks, msecs_to_jiffies() macro please! > > And please consider running checkpatch and sparse over your code. Sparse > gives, for example: > > drivers/i2c/busses/i2c-ibm_iic.c:418:24: warning: incorrect type in argument > 1 (different address spaces) > drivers/i2c/busses/i2c-ibm_iic.c:418:24:expected unsigned char const > volatile [noderef] [usertype] *addr > drivers/i2c/busses/i2c-ibm_iic.c:418:24:got unsigned char * > > (This probably due to patch 1 or 2, I'd guess) > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote: > + /* SoC ID */ > + soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16; > + > + /* SoC revision */ > + soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF) > + & SOC_REV_MASK; Sort of a minor nit, but I'm not actually sure these registers are read-only :( I know the documentation doesn't describe how to change them, but in PCI-E EndPoint mode they are read by the host so the firmware must be able to change them to reflect the firmware running on the endpoint.. Which is to say, I suppose the bootloader could program them to something else if it wanted to. That said, in practice obviously they will not be changed, but maybe there is another ID register you can read? Jason -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
Dear Gregory CLEMENT, On Fri, 3 Jan 2014 10:59:45 +0100, Gregory CLEMENT wrote: > + /* > + * Only revison more recent than A0 support offload revison -> revisions offload mechanism -> the offload mechanism > + * mechanism. In case we can't get the SoC revision > + * weplay safe and we don't enable it weplay -> we play > + */ > + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV) > + drv_data->offload_enabled = true; > } > > out: Thanks! Thomas -- Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
Dear Gregory CLEMENT,
On Fri, 3 Jan 2014 10:59:44 +0100, Gregory CLEMENT wrote:
> +static int __init mvebu_soc_id_init(void)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
> + if (np) {
Change this to:
if (!np)
return 0;
so that you avoid indenting the entire function code inside the if
{ ... } block.
> + void __iomem *pci_base;
> + struct clk *clk;
> + /*
> + * ID and revision are available from any port, so we
> + * just pick the first one
> + */
> + struct device_node *child = of_get_next_child(np, NULL);
Maybe check that child is not NULL here?
> +
> + clk = of_clk_get_by_name(child, NULL);
> + if (IS_ERR(clk)) {
> + pr_err("%s: cannot get clock\n", __func__);
I think you should rather do:
#define pr_fmt(fmt) "mvebu-soc-id: " fmt
at the beginning of your C file and get rid of the "%s" for the
__func__.
> + /* SoC ID */
> + soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
> +
> + /* SoC revision */
> + soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
> + & SOC_REV_MASK;
Use readl() for both of these reads. __raw_readl() will not work
properly when the system is booted big-endian.
> + return ret;
> +}
> +arch_initcall(mvebu_soc_id_init);
> +#ifdef CONFIG_ARCH_MVEBU
> +int mvebu_get_soc_id(u32 *dev, u32 *rev);
> +#else
> +int mvebu_get_soc_id(u32 *dev, u32 *rev)
Missing "static inline". Without these, if this header file is included
more than once, you will have two times the same symbol defined.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
> >> Hi Gregory
> >>
> >> I'm away from my hardware at the moment.
> >>
> >> Does this work when all the PCIe ports have status = "disabled";? We
> >> have many kirkwood devices in NAS boxes which don't use PCIe, so all
> >> the ports are disabled. But they still exist in the SoC, so we can
> >> read the IDs from them. I just don't know if of_get_next_child() will
> >> only return enabled children?
> >
> > There is a function named of_get_next_available_child, so I assumed that
> > of_get_next_child() will return all the children. But I can test it to
> > be sure of it.
> >
>
> I have just removed all the PCIe part in the
> armada-xp-openblocks-ax3-4.dts file (PCie is disable by default in the
> dtsi file) and it worled as expected! :)
Great, thanks for testing.
> by the way waht do you think of adding this line in at the end of the
> mvebu_soc_id_init() function:
>
> pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
Kirkwood prints actual strings, not numbers. More readable.
> Also keep in mind that currently you can't use it for kirkwood because
> the build of the file depend on CONFIG_ARCH_MVEBU. But as kirkwood
> will soon joined the mach-mvebu directory, it won't be a problem then.
I think it is possible to do ../mach-mvebu/soc_id.o sort of thing in
the Makefile. Ugly, but might work until we move.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: imx: propagate irq error code in probe
On Thu, Dec 12, 2013 at 10:51:31PM +0100, Wolfram Sang wrote: > smatch rightfully says: > drivers/i2c/busses/i2c-imx.c:610 i2c_imx_probe() info: why not propagate > 'irq' from platform_get_irq() instead of (-2)? > > Signed-off-by: Wolfram Sang Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v4] i2c: s3c2410: dont need CPU_FREQ transitions for exynos series
On Tue, Nov 26, 2013 at 09:52:46AM +0530, Naveen Krishna Chatradhi wrote: > For Exynos4 and Exynos5 SoCs from Samsung the i2c clock is based > on a fixed 66 MHz peripheral clock, and therefore is completely > independent of the cpu frequency. > Thus, registering for a CPU freq notifier is very wasteful. > > This patch modifes the code such that, i2c bus registers to > cpu_freq_transition only if CONFIG_CPU_FREQ_S3C24XX is enabled. > > This change should save a bunch of cpufreq transitions calls > which does not apply to exynos SoCs. > > Signed-off-by: Naveen Krishna Chatradhi > Acked-by: Kyungmin Park > Reviewed-by: Doug Anderson Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH V2] i2c: s3c2410 : Add polling mode support
Hi, On Mon, Nov 11, 2013 at 04:50:20PM +0530, Yuvaraj Kumar C D wrote: > From: Vasanth Ananthan > > This patch adds polling mode support for i2c-s3c2410 driver.The > SATA PHY controller's CMU and TRSV block's are of I2C register > map in exynos5250.These blocks can be configured using i2c. > > But i2c controller instance on which these block's sits lacks an > interrupt line.Also the current i2c-s3c2410 driver is only interrupt > driven, thus a polling mode support is required in the driver for > supporting this controller. This patch adds this support to the driver. > > Changes from V1: > 1.Changed the is_ack() to have even period b/w polls and > used usleep_range() instead of udelay(). Mileages vary, but I'd like to see revision changes after the "---". > ret = devm_request_irq(&pdev->dev, i2c->irq, s3c24xx_i2c_irq, 0, > -dev_name(&pdev->dev), i2c); > + dev_name(&pdev->dev), i2c); Unrelated change. Rest looks good, so I'll fix up the things for you and apply to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH v3 REPOST 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler
2014/1/3 Wolfram Sang :
>
> Hi,
>
> thanks for the submission!
You're welcome :) Thanks for reviewing this.
>
>> --- a/drivers/i2c/busses/i2c-ibm_iic.c
>> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
>> @@ -58,6 +58,8 @@ static bool iic_force_fast;
>> module_param(iic_force_fast, bool, 0);
>> MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)");
>>
>> +#define FIFO_FLUSH_TIMEOUT 100
>
> 100 what? The unit is missing.
The actual value of this timeout isn't very important. This is used
only to avoid a never ending loop in case the hardware goes into a
really bad state.
>
>> @@ -167,8 +170,8 @@ static void iic_dev_init(struct ibm_iic_private* dev)
>> /* Clear control register */
>> out_8(&iic->cntl, 0);
>>
>> - /* Enable interrupts if possible */
>> - iic_interrupt_mode(dev, dev->irq >= 0);
>> + /* Start with each individual interrupt masked*/
>
> Space at the end of comment missing
ok
>
>> static irqreturn_t iic_handler(int irq, void *dev_id)
>> {
>> - struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id;
>> - struct iic_regs __iomem *iic = dev->vaddr;
>> -
>> - DBG2(dev, "irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
>> - in_8(&iic->sts), in_8(&iic->extsts));
>> -
>> - /* Acknowledge IRQ and wakeup iic_wait_for_tc */
>> - out_8(&iic->sts, STS_IRQA | STS_SCMP);
>> - wake_up_interruptible(&dev->wq);
>> -
>> + struct ibm_iic_private *dev = (struct ibm_iic_private *) dev_id;
>> + iic_xfer_bytes(dev);
>
> Is iic_xfer_bytes later used when polling, too? Otherwise it could be
> simply inserted here.
Yes it'll be used for polling (implemented in a later patch)
>
>
>> + if ((status & STS_ERR) ||
>> + (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) {
>> + DBG(dev, "status 0x%x\n", status);
>> + DBG(dev, "extended status 0x%x\n", ext_status);
>> + if (status & STS_ERR)
>> + ERR(dev, "Error detected\n");
>> + if (ext_status & EXTSTS_LA)
>> + DBG(dev, "Lost arbitration\n");
>> + if (ext_status & EXTSTS_ICT)
>> + ERR(dev, "Incomplete transfer\n");
>> + if (ext_status & EXTSTS_XFRA)
>> + ERR(dev, "Transfer aborted\n");
>> +
>> + dev->status = -EIO;
>
> You could consider returning different fault codes for the different
> states. See Documentation/i2c/fault-codes for a guide.
I'll have a look at it.
>
>> + if (dev->msgs == NULL) {
>> + DBG(dev, "spurious !\n");
>> + dev->status = -EINVAL;
>> + return dev->status;
>> + }
>
> Does that really happen?
Not in my test cases (going through i2c dev). But it could if the data
passed to i2c_transfer is malformed.
Should I remove this ?
>
> And it introduces a build warning:
>
> drivers/i2c/busses/i2c-ibm_iic.c:410:12: warning: 'iic_wait_for_tc' defined
> but not used [-Wunused-function]
>
I posted this some time ago, but I believe I kept this in to help git
produce a diff readable by a human.
It's removed in a later patch
Jean-Jacques
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c-designware-pci: Index Haswell ULT bus names from 0
On Tue, Nov 26, 2013 at 02:09:59PM +0100, Wolfram Sang wrote: > On Tue, Nov 19, 2013 at 06:14:18PM -0800, Benson Leung wrote: > > Hi Wolfram, > > > > On Thu, Nov 14, 2013 at 10:05 AM, Wolfram Sang wrote: > > >> In the chromeos_laptop driver, I do by-name matching of i2c busses to > > >> find busses and instantiate devices, so there is value to have each > > >> named something predictable. > > > > > > Any why don't you use fixed bus numbers which you can attach the devices > > > to? > > > > On this particular set of systems, there are two other classes of i2c > > adapters that use dynamically assigned bus numbers, specifically the > > i915 gmbus adapters, and the i801_smbus adapter. This is why > > chromeos_laptop uses the name matching, as some of the boards that it > > supports have devices on those dynamic busses. > > I am not sure I get the problem. If you use i2c_register_board_info() to > register the known devices on the designware busses the dynamically > assigned numbers are guaranteed to be enumarated higer than the static > ones. Check drivers/i2c/i2c-boardinfo.c. Ping. Was this helpful or do you still have the issue? signature.asc Description: Digital signature
Re: [PATCH 01/15] i2c: shmobile/rcar: Restrict non-COMPILE_TEST compilation
On Wed, Nov 27, 2013 at 02:18:23AM +0100, Laurent Pinchart wrote: > Hardware supported by the i2c sh_mobile and rcar drivers is only found > on SUPERH or ARCH_SHMOBILE platforms. Restrict non-COMPILE_TEST > compilation to them. > > Cc: Wolfram Sang > Cc: linux-i2c@vger.kernel.org > Signed-off-by: Laurent Pinchart Applied to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH 0/4] (Repost) Add reset GPIO support to the pca954x I2C mux
On Fri, Nov 29, 2013 at 01:51:21AM +0100, Laurent Pinchart wrote: > Hello, > > This patch set adds DT support for a reset GPIO to the pca954x I2C mux. It > starts by two cleanup patches for the driver, adds missing DT bindings > documentation and finally adds support for the reset GPIO. Applied series to for-next, thanks! signature.asc Description: Digital signature
Re: [PATCH 3/3] i2c: nomadik: factor platform data into state container
> + if (!np) {
> + dev_err(&adev->dev, "no device node\n");
> + return -ENODEV;
Can this really happen? How should driver and device match otherwise?
Rest is looking good.
signature.asc
Description: Digital signature
Re: [PATCH v3 REPOST 4/4] i2c: i2c-ibm-iic: Implements a polling mode
On Fri, Dec 20, 2013 at 04:12:56PM +0100, jean-jacques hiblot wrote: > From: jean-jacques hiblot > > When no valid interrupt is defined for the controller, use polling to handle > the transfers. > The polling mode can also be forced with the "iic_force_poll" module > parameter. > > Signed-off-by: jean-jacques hiblot Do I understand correctly: Polling was not available after patch 2 until patch 4? Has the signal handling been tested extensively? Most drivers do not handle signals because it is usually cumbersome to cancel a transfer gracefully. That being said, it might work well for you here. signature.asc Description: Digital signature
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
Hi Andrew,
On 03/01/2014 15:51, Gregory CLEMENT wrote:
> On 03/01/2014 15:47, Andrew Lunn wrote:
>> On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
>>> All the mvebu SoCs have information related to their variant and
>>> revision that can be read from the PCI control register.
>>>
>>> This patch adds support for Armada XP and Armada 370. This reading of
>>> the revision and the ID are done before the PCI initialization to
>>> avoid any conflicts. Once these data are retrieved, the resources are
>>> freed to let the PCI subsystem use it.
>>>
>>> Cc: sta...@vger.kernel.org
>>> Signed-off-by: Gregory CLEMENT
>>> ---
>>> arch/arm/mach-mvebu/Makefile | 2 +-
>>> arch/arm/mach-mvebu/mvebu-soc-id.c | 111
>>> +
>>> include/linux/mvebu-soc-id.h | 32 +++
>>> 3 files changed, 144 insertions(+), 1 deletion(-)
>>> create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
>>> create mode 100644 include/linux/mvebu-soc-id.h
>>>
>>> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
>>> index 2d04f0e21870..878aebe98dcc 100644
>>> --- a/arch/arm/mach-mvebu/Makefile
>>> +++ b/arch/arm/mach-mvebu/Makefile
>>> @@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) :=
>>> -I$(srctree)/$(src)/include \
>>>
>>> AFLAGS_coherency_ll.o := -Wa,-march=armv7-a
>>>
>>> -obj-y += system-controller.o
>>> +obj-y += system-controller.o mvebu-soc-id.o
>>> obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
>>> obj-$(CONFIG_ARCH_MVEBU)+= coherency.o coherency_ll.o pmsu.o
>>> obj-$(CONFIG_SMP)+= platsmp.o headsmp.o
>>> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c
>>> b/arch/arm/mach-mvebu/mvebu-soc-id.c
>>> new file mode 100644
>>> index ..86c901be284c
>>> --- /dev/null
>>> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
>>> @@ -0,0 +1,111 @@
>>> +/*
>>> + * Variant and revision information for mvebu SoCs
>>> + *
>>> + * Copyright (C) 2014 Marvell
>>> + *
>>> + * Gregory CLEMENT
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public
>>> + * License version 2. This program is licensed "as is" without any
>>> + * warranty of any kind, whether express or implied.
>>> + *
>>> + * All the mvebu SoCs have information related to their variant and
>>> + * revision that can be read from the PCI control register. This is
>>> + * done before the PCI initialization to avoid any conflict. Once the
>>> + * ID and revision are retrieved, the mapping is freed.
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#define PCIE_DEV_ID_OFF0x0
>>> +#define PCIE_DEV_REV_OFF 0x8
>>> +
>>> +#define SOC_ID_MASK0x
>>> +#define SOC_REV_MASK 0xFF
>>> +
>>> +static u32 soc_dev_id;
>>> +static u32 soc_rev;
>>> +static bool is_id_valid;
>>> +
>>> +static const struct of_device_id mvebu_pcie_of_match_table[] = {
>>> + { .compatible = "marvell,armada-xp-pcie", },
>>> + { .compatible = "marvell,armada-370-pcie", },
>>> + {},
>>> +};
>>> +
>>> +int mvebu_get_soc_id(u32 *dev, u32 *rev)
>>> +{
>>> + if (is_id_valid) {
>>> + *dev = soc_dev_id;
>>> + *rev = soc_rev;
>>> + return 0;
>>> + } else
>>> + return -1;
>>> +}
>>> +
>>> +EXPORT_SYMBOL(mvebu_get_soc_id);
>>> +
>>> +static int __init mvebu_soc_id_init(void)
>>> +{
>>> + struct device_node *np;
>>> + int ret = 0;
>>> +
>>> + np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
>>> + if (np) {
>>> + void __iomem *pci_base;
>>> + struct clk *clk;
>>> + /*
>>> +* ID and revision are available from any port, so we
>>> +* just pick the first one
>>> +*/
>>> + struct device_node *child = of_get_next_child(np, NULL);
>>
>> Hi Gregory
>>
>> I'm away from my hardware at the moment.
>>
>> Does this work when all the PCIe ports have status = "disabled";? We
>> have many kirkwood devices in NAS boxes which don't use PCIe, so all
>> the ports are disabled. But they still exist in the SoC, so we can
>> read the IDs from them. I just don't know if of_get_next_child() will
>> only return enabled children?
>
> There is a function named of_get_next_available_child, so I assumed that
> of_get_next_child() will return all the children. But I can test it to
> be sure of it.
>
I have just removed all the PCIe part in the
armada-xp-openblocks-ax3-4.dts file (PCie is disable by default in the
dtsi file) and it worled as expected! :)
by the way waht do you think of adding this line in at the end of the
mvebu_soc_id_init() function:
pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
Also keep in mind that currently you can't use it for kirkwood because
the build of the file depend on CONFIG_ARCH_MVEBU. But as kirkwood
will soon joined the m
Re: [PATCH v3 REPOST 3/4] i2c: i2c-ibm-iic: Implements transfer abortion
On Fri, Dec 20, 2013 at 04:12:55PM +0100, jean-jacques hiblot wrote: > From: jean-jacques hiblot > > Clean-up properly when a transfer fails for whatever reason. > Cancel the transfer when the process is signaled. Please describe what you do a little. I wonder how you can remove so much code while keeping the functionality? > > Signed-off-by: jean-jacques hiblot > - out_8(&iic->cntl, CNTL_HMT); > + DBG(dev, "aborting transfer\n"); > + /* transfer should be aborted within 10ms */ > + end = jiffies + 10; Eeks, msecs_to_jiffies() macro please! And please consider running checkpatch and sparse over your code. Sparse gives, for example: drivers/i2c/busses/i2c-ibm_iic.c:418:24: warning: incorrect type in argument 1 (different address spaces) drivers/i2c/busses/i2c-ibm_iic.c:418:24:expected unsigned char const volatile [noderef] [usertype] *addr drivers/i2c/busses/i2c-ibm_iic.c:418:24:got unsigned char * (This probably due to patch 1 or 2, I'd guess) signature.asc Description: Digital signature
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On 03/01/2014 15:47, Andrew Lunn wrote:
> On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
>> All the mvebu SoCs have information related to their variant and
>> revision that can be read from the PCI control register.
>>
>> This patch adds support for Armada XP and Armada 370. This reading of
>> the revision and the ID are done before the PCI initialization to
>> avoid any conflicts. Once these data are retrieved, the resources are
>> freed to let the PCI subsystem use it.
>>
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gregory CLEMENT
>> ---
>> arch/arm/mach-mvebu/Makefile | 2 +-
>> arch/arm/mach-mvebu/mvebu-soc-id.c | 111
>> +
>> include/linux/mvebu-soc-id.h | 32 +++
>> 3 files changed, 144 insertions(+), 1 deletion(-)
>> create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
>> create mode 100644 include/linux/mvebu-soc-id.h
>>
>> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
>> index 2d04f0e21870..878aebe98dcc 100644
>> --- a/arch/arm/mach-mvebu/Makefile
>> +++ b/arch/arm/mach-mvebu/Makefile
>> @@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) :=
>> -I$(srctree)/$(src)/include \
>>
>> AFLAGS_coherency_ll.o := -Wa,-march=armv7-a
>>
>> -obj-y+= system-controller.o
>> +obj-y+= system-controller.o mvebu-soc-id.o
>> obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
>> obj-$(CONFIG_ARCH_MVEBU) += coherency.o coherency_ll.o pmsu.o
>> obj-$(CONFIG_SMP)+= platsmp.o headsmp.o
>> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c
>> b/arch/arm/mach-mvebu/mvebu-soc-id.c
>> new file mode 100644
>> index ..86c901be284c
>> --- /dev/null
>> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * Variant and revision information for mvebu SoCs
>> + *
>> + * Copyright (C) 2014 Marvell
>> + *
>> + * Gregory CLEMENT
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + *
>> + * All the mvebu SoCs have information related to their variant and
>> + * revision that can be read from the PCI control register. This is
>> + * done before the PCI initialization to avoid any conflict. Once the
>> + * ID and revision are retrieved, the mapping is freed.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define PCIE_DEV_ID_OFF 0x0
>> +#define PCIE_DEV_REV_OFF0x8
>> +
>> +#define SOC_ID_MASK 0x
>> +#define SOC_REV_MASK0xFF
>> +
>> +static u32 soc_dev_id;
>> +static u32 soc_rev;
>> +static bool is_id_valid;
>> +
>> +static const struct of_device_id mvebu_pcie_of_match_table[] = {
>> +{ .compatible = "marvell,armada-xp-pcie", },
>> +{ .compatible = "marvell,armada-370-pcie", },
>> +{},
>> +};
>> +
>> +int mvebu_get_soc_id(u32 *dev, u32 *rev)
>> +{
>> +if (is_id_valid) {
>> +*dev = soc_dev_id;
>> +*rev = soc_rev;
>> +return 0;
>> +} else
>> +return -1;
>> +}
>> +
>> +EXPORT_SYMBOL(mvebu_get_soc_id);
>> +
>> +static int __init mvebu_soc_id_init(void)
>> +{
>> +struct device_node *np;
>> +int ret = 0;
>> +
>> +np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
>> +if (np) {
>> +void __iomem *pci_base;
>> +struct clk *clk;
>> +/*
>> + * ID and revision are available from any port, so we
>> + * just pick the first one
>> + */
>> +struct device_node *child = of_get_next_child(np, NULL);
>
> Hi Gregory
>
> I'm away from my hardware at the moment.
>
> Does this work when all the PCIe ports have status = "disabled";? We
> have many kirkwood devices in NAS boxes which don't use PCIe, so all
> the ports are disabled. But they still exist in the SoC, so we can
> read the IDs from them. I just don't know if of_get_next_child() will
> only return enabled children?
There is a function named of_get_next_available_child, so I assumed that
of_get_next_child() will return all the children. But I can test it to
be sure of it.
>
> Thanks
> Andrew
>
>> +
>> +clk = of_clk_get_by_name(child, NULL);
>> +if (IS_ERR(clk)) {
>> +pr_err("%s: cannot get clock\n", __func__);
>> +ret = -ENOMEM;
>> +goto clk_err;
>> +}
>> +
>> +ret = clk_prepare_enable(clk);
>> +if (ret) {
>> +pr_err("%s: cannot enable clock\n", __func__);
>> +goto clk_err;
>> +}
>> +
>> +pci_base = of_iomap(child, 0);
>> +if (IS_ERR(pci_base)) {
>> +pr_err("%s: cannot map registers\n",
Re: [PATCH v3 REPOST 2/4] i2c: i2c-ibm-iic: perform the transfer in the interrupt handler
Hi,
thanks for the submission!
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -58,6 +58,8 @@ static bool iic_force_fast;
> module_param(iic_force_fast, bool, 0);
> MODULE_PARM_DESC(iic_force_fast, "Force fast mode (400 kHz)");
>
> +#define FIFO_FLUSH_TIMEOUT 100
100 what? The unit is missing.
> @@ -167,8 +170,8 @@ static void iic_dev_init(struct ibm_iic_private* dev)
> /* Clear control register */
> out_8(&iic->cntl, 0);
>
> - /* Enable interrupts if possible */
> - iic_interrupt_mode(dev, dev->irq >= 0);
> + /* Start with each individual interrupt masked*/
Space at the end of comment missing
> static irqreturn_t iic_handler(int irq, void *dev_id)
> {
> - struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_id;
> - struct iic_regs __iomem *iic = dev->vaddr;
> -
> - DBG2(dev, "irq handler, STS = 0x%02x, EXTSTS = 0x%02x\n",
> - in_8(&iic->sts), in_8(&iic->extsts));
> -
> - /* Acknowledge IRQ and wakeup iic_wait_for_tc */
> - out_8(&iic->sts, STS_IRQA | STS_SCMP);
> - wake_up_interruptible(&dev->wq);
> -
> + struct ibm_iic_private *dev = (struct ibm_iic_private *) dev_id;
> + iic_xfer_bytes(dev);
Is iic_xfer_bytes later used when polling, too? Otherwise it could be
simply inserted here.
> + if ((status & STS_ERR) ||
> + (ext_status & (EXTSTS_LA | EXTSTS_ICT | EXTSTS_XFRA))) {
> + DBG(dev, "status 0x%x\n", status);
> + DBG(dev, "extended status 0x%x\n", ext_status);
> + if (status & STS_ERR)
> + ERR(dev, "Error detected\n");
> + if (ext_status & EXTSTS_LA)
> + DBG(dev, "Lost arbitration\n");
> + if (ext_status & EXTSTS_ICT)
> + ERR(dev, "Incomplete transfer\n");
> + if (ext_status & EXTSTS_XFRA)
> + ERR(dev, "Transfer aborted\n");
> +
> + dev->status = -EIO;
You could consider returning different fault codes for the different
states. See Documentation/i2c/fault-codes for a guide.
> + if (dev->msgs == NULL) {
> + DBG(dev, "spurious !\n");
> + dev->status = -EINVAL;
> + return dev->status;
> + }
Does that really happen?
And it introduces a build warning:
drivers/i2c/busses/i2c-ibm_iic.c:410:12: warning: 'iic_wait_for_tc' defined but
not used [-Wunused-function]
signature.asc
Description: Digital signature
Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
On Fri, Jan 03, 2014 at 10:59:44AM +0100, Gregory CLEMENT wrote:
> All the mvebu SoCs have information related to their variant and
> revision that can be read from the PCI control register.
>
> This patch adds support for Armada XP and Armada 370. This reading of
> the revision and the ID are done before the PCI initialization to
> avoid any conflicts. Once these data are retrieved, the resources are
> freed to let the PCI subsystem use it.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Gregory CLEMENT
> ---
> arch/arm/mach-mvebu/Makefile | 2 +-
> arch/arm/mach-mvebu/mvebu-soc-id.c | 111
> +
> include/linux/mvebu-soc-id.h | 32 +++
> 3 files changed, 144 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
> create mode 100644 include/linux/mvebu-soc-id.h
>
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 2d04f0e21870..878aebe98dcc 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) :=
> -I$(srctree)/$(src)/include \
>
> AFLAGS_coherency_ll.o:= -Wa,-march=armv7-a
>
> -obj-y += system-controller.o
> +obj-y += system-controller.o mvebu-soc-id.o
> obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
> obj-$(CONFIG_ARCH_MVEBU) += coherency.o coherency_ll.o pmsu.o
> obj-$(CONFIG_SMP)+= platsmp.o headsmp.o
> diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c
> b/arch/arm/mach-mvebu/mvebu-soc-id.c
> new file mode 100644
> index ..86c901be284c
> --- /dev/null
> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
> @@ -0,0 +1,111 @@
> +/*
> + * Variant and revision information for mvebu SoCs
> + *
> + * Copyright (C) 2014 Marvell
> + *
> + * Gregory CLEMENT
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + *
> + * All the mvebu SoCs have information related to their variant and
> + * revision that can be read from the PCI control register. This is
> + * done before the PCI initialization to avoid any conflict. Once the
> + * ID and revision are retrieved, the mapping is freed.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define PCIE_DEV_ID_OFF 0x0
> +#define PCIE_DEV_REV_OFF 0x8
> +
> +#define SOC_ID_MASK 0x
> +#define SOC_REV_MASK 0xFF
> +
> +static u32 soc_dev_id;
> +static u32 soc_rev;
> +static bool is_id_valid;
> +
> +static const struct of_device_id mvebu_pcie_of_match_table[] = {
> + { .compatible = "marvell,armada-xp-pcie", },
> + { .compatible = "marvell,armada-370-pcie", },
> + {},
> +};
> +
> +int mvebu_get_soc_id(u32 *dev, u32 *rev)
> +{
> + if (is_id_valid) {
> + *dev = soc_dev_id;
> + *rev = soc_rev;
> + return 0;
> + } else
> + return -1;
> +}
> +
> +EXPORT_SYMBOL(mvebu_get_soc_id);
> +
> +static int __init mvebu_soc_id_init(void)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
> + if (np) {
> + void __iomem *pci_base;
> + struct clk *clk;
> + /*
> + * ID and revision are available from any port, so we
> + * just pick the first one
> + */
> + struct device_node *child = of_get_next_child(np, NULL);
Hi Gregory
I'm away from my hardware at the moment.
Does this work when all the PCIe ports have status = "disabled";? We
have many kirkwood devices in NAS boxes which don't use PCIe, so all
the ports are disabled. But they still exist in the SoC, so we can
read the IDs from them. I just don't know if of_get_next_child() will
only return enabled children?
Thanks
Andrew
> +
> + clk = of_clk_get_by_name(child, NULL);
> + if (IS_ERR(clk)) {
> + pr_err("%s: cannot get clock\n", __func__);
> + ret = -ENOMEM;
> + goto clk_err;
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + pr_err("%s: cannot enable clock\n", __func__);
> + goto clk_err;
> + }
> +
> + pci_base = of_iomap(child, 0);
> + if (IS_ERR(pci_base)) {
> + pr_err("%s: cannot map registers\n", __func__);
> + ret = -ENOMEM;
> + goto res_ioremap;
> + }
> +
> + /* SoC ID */
> + soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
> +
> + /* SoC revision */
> + soc_rev = __raw_readl(pci_base + PCIE_DEV
Re: [PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
Hi Jason,
On 03/01/2014 10:59, Gregory CLEMENT wrote:
> The first variants of Armada XP SoCs (A0 stepping) have issues related
> to the i2c controller which prevent to use the offload mechanism and
> lead to a kernel hang during boot.
>
> The driver now check the revision of the SoC. If the revision is not
> more recent than the A0 or if the driver can't get the SoC revision
> then it disables the offload mechanism.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Gregory CLEMENT
> Acked-by: Wolfram Sang
I have just realized that it should be fair to add a
Reported-by: Andrew Lunn
Thanks,
Gregory
> ---
> drivers/i2c/busses/i2c-mv64xxx.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
> b/drivers/i2c/busses/i2c-mv64xxx.c
> index 8be7e42aa4de..634b04d241fb 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
>
> #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1)
> #define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7)
> @@ -779,8 +780,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>* Transaction Generator support and the errata fix.
>*/
> if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
> - drv_data->offload_enabled = true;
> + u32 dev, rev;
> +
> drv_data->errata_delay = true;
> + /*
> + * Only revison more recent than A0 support offload
> + * mechanism. In case we can't get the SoC revision
> + * weplay safe and we don't enable it
> + */
> + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> + drv_data->offload_enabled = true;
> }
>
> out:
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC
All the mvebu SoCs have information related to their variant and
revision that can be read from the PCI control register.
This patch adds support for Armada XP and Armada 370. This reading of
the revision and the ID are done before the PCI initialization to
avoid any conflicts. Once these data are retrieved, the resources are
freed to let the PCI subsystem use it.
Cc: sta...@vger.kernel.org
Signed-off-by: Gregory CLEMENT
---
arch/arm/mach-mvebu/Makefile | 2 +-
arch/arm/mach-mvebu/mvebu-soc-id.c | 111 +
include/linux/mvebu-soc-id.h | 32 +++
3 files changed, 144 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
create mode 100644 include/linux/mvebu-soc-id.h
diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 2d04f0e21870..878aebe98dcc 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) :=
-I$(srctree)/$(src)/include \
AFLAGS_coherency_ll.o := -Wa,-march=armv7-a
-obj-y += system-controller.o
+obj-y += system-controller.o mvebu-soc-id.o
obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
obj-$(CONFIG_ARCH_MVEBU)+= coherency.o coherency_ll.o pmsu.o
obj-$(CONFIG_SMP)+= platsmp.o headsmp.o
diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c
b/arch/arm/mach-mvebu/mvebu-soc-id.c
new file mode 100644
index ..86c901be284c
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -0,0 +1,111 @@
+/*
+ * Variant and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ *
+ * All the mvebu SoCs have information related to their variant and
+ * revision that can be read from the PCI control register. This is
+ * done before the PCI initialization to avoid any conflict. Once the
+ * ID and revision are retrieved, the mapping is freed.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define PCIE_DEV_ID_OFF0x0
+#define PCIE_DEV_REV_OFF 0x8
+
+#define SOC_ID_MASK0x
+#define SOC_REV_MASK 0xFF
+
+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;
+
+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+ { .compatible = "marvell,armada-xp-pcie", },
+ { .compatible = "marvell,armada-370-pcie", },
+ {},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+ if (is_id_valid) {
+ *dev = soc_dev_id;
+ *rev = soc_rev;
+ return 0;
+ } else
+ return -1;
+}
+
+EXPORT_SYMBOL(mvebu_get_soc_id);
+
+static int __init mvebu_soc_id_init(void)
+{
+ struct device_node *np;
+ int ret = 0;
+
+ np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+ if (np) {
+ void __iomem *pci_base;
+ struct clk *clk;
+ /*
+* ID and revision are available from any port, so we
+* just pick the first one
+*/
+ struct device_node *child = of_get_next_child(np, NULL);
+
+ clk = of_clk_get_by_name(child, NULL);
+ if (IS_ERR(clk)) {
+ pr_err("%s: cannot get clock\n", __func__);
+ ret = -ENOMEM;
+ goto clk_err;
+ }
+
+ ret = clk_prepare_enable(clk);
+ if (ret) {
+ pr_err("%s: cannot enable clock\n", __func__);
+ goto clk_err;
+ }
+
+ pci_base = of_iomap(child, 0);
+ if (IS_ERR(pci_base)) {
+ pr_err("%s: cannot map registers\n", __func__);
+ ret = -ENOMEM;
+ goto res_ioremap;
+ }
+
+ /* SoC ID */
+ soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+ /* SoC revision */
+ soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+ & SOC_REV_MASK;
+
+ is_id_valid = true;
+
+ iounmap(pci_base);
+
+res_ioremap:
+ clk_disable_unprepare(clk);
+
+clk_err:
+ of_node_put(child);
+ of_node_put(np);
+ }
+
+ return ret;
+}
+arch_initcall(mvebu_soc_id_init);
+
diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
new file mode 100644
index ..602ce1c50d1d
--- /dev/null
+++ b/include/linux/mvebu-soc-id.h
@@ -0,0 +1,32 @@
+/*
+ * Marvell EBU SoC ID and revision definitions.
+ *
+ * Copyright (C) 2014 Marvell Semiconductor
+ *
+ * Th
[PATCH v2 0/2] Fix i2c bus hang on A0 version of the Armada XP SoCs
Hi, This the 2nd version of the series fixing the i2c bus hang on A0 version of the Armada XP SoCs. It occurred on the early release of the OpenBlocks AX3-4 boards. Indeed the first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot. The first patch add a mean to detect the SoCs version at run-time and the second one use this feature in the driver. These 2 patches should be applied on 3.13-rc and on stable kernel 3.12 as it fixes a regression introduce by the commit 930ab3d403ae "i2c: mv64xxx: Add I2C Transaction Generator support". The first patch could be latter be extend to also be used with dove, kirkwood, orion5x and mv78x00 when there will be merged in mvebu. Thanks, Gregory Changelog: v1 -> v2: - Changed the way to test the return of the function mvebu_get_soc_id in order to make it clearer. - Removed the superfluous parentheses - Added Wolfram's acked-by on the 2nd patch Gregory CLEMENT (2): ARM: mvebu: Add support to get the ID and the revision of a SoC i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs arch/arm/mach-mvebu/Makefile | 2 +- arch/arm/mach-mvebu/mvebu-soc-id.c | 111 + drivers/i2c/busses/i2c-mv64xxx.c | 11 +++- include/linux/mvebu-soc-id.h | 32 +++ 4 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c create mode 100644 include/linux/mvebu-soc-id.h -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
The first variants of Armada XP SoCs (A0 stepping) have issues related
to the i2c controller which prevent to use the offload mechanism and
lead to a kernel hang during boot.
The driver now check the revision of the SoC. If the revision is not
more recent than the A0 or if the driver can't get the SoC revision
then it disables the offload mechanism.
Cc: sta...@vger.kernel.org
Signed-off-by: Gregory CLEMENT
Acked-by: Wolfram Sang
---
drivers/i2c/busses/i2c-mv64xxx.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8be7e42aa4de..634b04d241fb 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1)
#define MV64XXX_I2C_BAUD_DIV_N(val)(val & 0x7)
@@ -779,8 +780,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
* Transaction Generator support and the errata fix.
*/
if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
- drv_data->offload_enabled = true;
+ u32 dev, rev;
+
drv_data->errata_delay = true;
+ /*
+* Only revison more recent than A0 support offload
+* mechanism. In case we can't get the SoC revision
+* weplay safe and we don't enable it
+*/
+ if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+ drv_data->offload_enabled = true;
}
out:
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
On 02/01/2014 19:41, Wolfram Sang wrote:
> On Thu, Jan 02, 2014 at 01:28:22PM -0500, Jason Cooper wrote:
>> Wolfram,
>>
>> On Thu, Jan 02, 2014 at 05:01:16PM +0100, Gregory CLEMENT wrote:
>>> The first variants of Armada XP SoCs (A0 stepping) have issues related
>>> to the i2c controller which prevent to use the offload mechanism and
>>> lead to a kernel hang during boot.
>>>
>>> The driver now check the revision of the SoC. If the revision is not
>>> more recent than the A0 or if the driver can't get the SoC revision
>>> then it disables the offload mechanism.
>>>
>>> Cc: sta...@vger.kernel.org
>>> Signed-off-by: Gregory CLEMENT
>>> ---
>>> drivers/i2c/busses/i2c-mv64xxx.c | 11 ++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c
>>> b/drivers/i2c/busses/i2c-mv64xxx.c
>>> index 8be7e42aa4de..089a3663ad86 100644
>>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>>> @@ -24,6 +24,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1)
>>> #define MV64XXX_I2C_BAUD_DIV_N(val)(val & 0x7)
>>> @@ -779,8 +780,16 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
>>> * Transaction Generator support and the errata fix.
>>> */
>>> if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
>>> - drv_data->offload_enabled = true;
>>> + u32 dev, rev;
>>> +
>>> drv_data->errata_delay = true;
>>> + /*
>>> +* Only revison more recent than A0 support offload
>>> +* mechanism. In case we can't get the SoC revision
>>> +* weplay safe and we don't enable it
>>> +*/
>>> + if (!mvebu_get_soc_id(&rev, &dev) && (dev > MV78XX0_A0_REV))
>
> Very minor nits:
>
> I'd prefer (mvebu_get_soc_id == 0) here, since !mvebu_get_soc_id can
> easily be read as "if not get soc id" which leads to the assumption the
> function failed.
yes fair enough
>And the parantheses around the second comparison are
> superfluous.
>
I know but I found it clearer with parenthesis but I can remove them.
>>> + drv_data->offload_enabled = true;
>>
>> Since this depends on arch-specific code in the previous patch, I'd like
>> to keep the two of them together in a topic branch. Would you prefer to
>> take both with my Ack, or vice-versa? I'm fine either way.
>
> I'd think you better take it:
>
> Acked-by: Wolfram Sang
>
I am going to resubmit a series with the change you asked and your acked-by
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
