| Message ID | 1390822488-22183-1-git-send-email-pranavkumar@linaro.org |
|---|---|
| State | Accepted |
| Commit | b88641348817bb9039ad81259681064ec4bfdb09 |
| Headers | show |
On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote: > This patch adds a reset support for xgene arm64 platform. > > V6: > - Incorporating comments received on V5 patch. > V5: > - Incorporating comments received on V4 patch. > V4: > - Removing TODO comment about retriving reset base address from dts > as that is done now. > V3: > - Retriving reset base address and reset mask from device tree. > - Removed unnecssary header files included earlier. > V2: > - Removed unnecssary mdelay in code. > - Adding iounmap of the base address. > V1: > -Initial patch. > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > Signed-off-by: Anup Patel <anup.patel@linaro.org> Looks good, thanks: Acked-by: Ian Campbell <ian.campbell@citrix.com> George, I'd like to put this in 4.4. The impact on non-xgene is non existent and I think reset is basic functionality which we should enable. > --- > xen/arch/arm/platforms/xgene-storm.c | 72 ++++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c > index 5b0bd5f..4fc185b 100644 > --- a/xen/arch/arm/platforms/xgene-storm.c > +++ b/xen/arch/arm/platforms/xgene-storm.c > @@ -20,8 +20,16 @@ > > #include <xen/config.h> > #include <asm/platform.h> > +#include <xen/stdbool.h> > +#include <xen/vmap.h> > +#include <asm/io.h> > #include <asm/gic.h> > > +/* Variables to save reset address of soc during platform initialization. */ > +static u64 reset_addr, reset_size; > +static u32 reset_mask; > +static bool reset_vals_valid = false; > + > static uint32_t xgene_storm_quirks(void) > { > return PLATFORM_QUIRK_GIC_64K_STRIDE; > @@ -107,6 +115,68 @@ err: > return ret; > } > > +static void xgene_storm_reset(void) > +{ > + void __iomem *addr; > + > + if ( !reset_vals_valid ) > + { > + printk("XGENE: Invalid reset values, can not reset XGENE...\n"); > + return; > + } > + > + addr = ioremap_nocache(reset_addr, reset_size); > + > + if ( !addr ) > + { > + printk("XGENE: Unable to map xgene reset address, can not reset XGENE...\n"); > + return; > + } > + > + /* Write reset mask to base address */ > + writel(reset_mask, addr); > + > + iounmap(addr); > +} > + > +static int xgene_storm_init(void) > +{ > + static const struct dt_device_match reset_ids[] __initconst = > + { > + DT_MATCH_COMPATIBLE("apm,xgene-reboot"), > + {}, > + }; > + struct dt_device_node *dev; > + int res; > + > + dev = dt_find_matching_node(NULL, reset_ids); > + if ( !dev ) > + { > + printk("XGENE: Unable to find a compatible reset node in the device tree"); > + return 0; > + } > + > + dt_device_set_used_by(dev, DOMID_XEN); > + > + /* Retrieve base address and size */ > + res = dt_device_get_address(dev, 0, &reset_addr, &reset_size); > + if ( res ) > + { > + printk("XGENE: Unable to retrieve the base address for reset\n"); > + return 0; > + } > + > + /* Get reset mask */ > + res = dt_property_read_u32(dev, "mask", &reset_mask); > + if ( !res ) > + { > + printk("XGENE: Unable to retrieve the reset mask\n"); > + return 0; > + } > + > + reset_vals_valid = true; > + return 0; > +} > > static const char * const xgene_storm_dt_compat[] __initconst = > { > @@ -116,6 +186,8 @@ static const char * const xgene_storm_dt_compat[] __initconst = > > PLATFORM_START(xgene_storm, "APM X-GENE STORM") > .compatible = xgene_storm_dt_compat, > + .init = xgene_storm_init, > + .reset = xgene_storm_reset, > .quirks = xgene_storm_quirks, > .specific_mapping = xgene_storm_specific_mapping, >
On Mon, 2014-01-27 at 12:01 +0000, George Dunlap wrote: > On 01/27/2014 11:38 AM, Ian Campbell wrote: > > On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote: > >> This patch adds a reset support for xgene arm64 platform. > >> > >> V6: > >> - Incorporating comments received on V5 patch. > >> V5: > >> - Incorporating comments received on V4 patch. > >> V4: > >> - Removing TODO comment about retriving reset base address from dts > >> as that is done now. > >> V3: > >> - Retriving reset base address and reset mask from device tree. > >> - Removed unnecssary header files included earlier. > >> V2: > >> - Removed unnecssary mdelay in code. > >> - Adding iounmap of the base address. > >> V1: > >> -Initial patch. > >> > >> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > >> Signed-off-by: Anup Patel <anup.patel@linaro.org> > > Looks good, thanks: > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > > > George, I'd like to put this in 4.4. The impact on non-xgene is non > > existent and I think reset is basic functionality which we should > > enable. > > So without this patch, support for xgene would be considered > "experimental" (missing basic functionality)? I don't think I would go so far as to say "experimental", we could probably live with it, but having to go find the board and press the tiny little button to reboot it is certainly not "complete". Ian.
On 01/27/2014 11:34 AM, Pranavkumar Sawargaonkar wrote: > This patch adds a reset support for xgene arm64 platform. > > V6: > - Incorporating comments received on V5 patch. > V5: > - Incorporating comments received on V4 patch. > V4: > - Removing TODO comment about retriving reset base address from dts > as that is done now. > V3: > - Retriving reset base address and reset mask from device tree. > - Removed unnecssary header files included earlier. > V2: > - Removed unnecssary mdelay in code. > - Adding iounmap of the base address. > V1: > -Initial patch. > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > Signed-off-by: Anup Patel <anup.patel@linaro.org> > --- > xen/arch/arm/platforms/xgene-storm.c | 72 ++++++++++++++++++++++++++++++++++ > 1 file changed, 72 insertions(+) > > diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c > index 5b0bd5f..4fc185b 100644 > --- a/xen/arch/arm/platforms/xgene-storm.c > +++ b/xen/arch/arm/platforms/xgene-storm.c > @@ -20,8 +20,16 @@ > > #include <xen/config.h> > #include <asm/platform.h> > +#include <xen/stdbool.h> > +#include <xen/vmap.h> > +#include <asm/io.h> > #include <asm/gic.h> > > +/* Variables to save reset address of soc during platform initialization. */ > +static u64 reset_addr, reset_size; > +static u32 reset_mask; > +static bool reset_vals_valid = false; > + > static uint32_t xgene_storm_quirks(void) > { > return PLATFORM_QUIRK_GIC_64K_STRIDE; > @@ -107,6 +115,68 @@ err: > return ret; > } > > +static void xgene_storm_reset(void) > +{ I'm concerned about reset function in general, in common code we have this code (arch/arm/shutdown.c machine_restart). while (1) { raw_machine_reset(); // which call platform_reset() mdelay(100); } If platform_reset failed, it's possible with this code, the console will be spam with "XGENE: ...". Do we really need to call raw_machine_reset multiple time? Would it be more suitable to have this code: raw_machine_reset(); mdelay(100); printk("Failed to reset\n"); while (1); Or even better, moving the mdelay per platform... > + void __iomem *addr; > + > + if ( !reset_vals_valid ) > + { > + printk("XGENE: Invalid reset values, can not reset XGENE...\n"); > + return; > + } > + > + addr = ioremap_nocache(reset_addr, reset_size); > + > + if ( !addr ) > + { > + printk("XGENE: Unable to map xgene reset address, can not reset XGENE...\n"); > + return; > + } > + > + /* Write reset mask to base address */ > + writel(reset_mask, addr); > + > + iounmap(addr); > +} > + > +static int xgene_storm_init(void) > +{ > + static const struct dt_device_match reset_ids[] __initconst = > + { > + DT_MATCH_COMPATIBLE("apm,xgene-reboot"), > + {}, Do you plan to have other ids in the future? If not, I would directly use dt_find_compatible_node(NULL, NULL "arm,xgene-reboot"); instead of dt_find_matching_node(...). > + }; > + struct dt_device_node *dev; > + int res; > + > + dev = dt_find_matching_node(NULL, reset_ids); > + if ( !dev ) > + { > + printk("XGENE: Unable to find a compatible reset node in the device tree"); > + return 0; > + } > + > + dt_device_set_used_by(dev, DOMID_XEN); > + > + /* Retrieve base address and size */ > + res = dt_device_get_address(dev, 0, &reset_addr, &reset_size); > + if ( res ) > + { > + printk("XGENE: Unable to retrieve the base address for reset\n"); > + return 0; > + } > + > + /* Get reset mask */ > + res = dt_property_read_u32(dev, "mask", &reset_mask); > + if ( !res ) > + { > + printk("XGENE: Unable to retrieve the reset mask\n"); > + return 0; > + } > + > + reset_vals_valid = true; > + return 0; > +} > > static const char * const xgene_storm_dt_compat[] __initconst = > { > @@ -116,6 +186,8 @@ static const char * const xgene_storm_dt_compat[] __initconst = > > PLATFORM_START(xgene_storm, "APM X-GENE STORM") > .compatible = xgene_storm_dt_compat, > + .init = xgene_storm_init, > + .reset = xgene_storm_reset, > .quirks = xgene_storm_quirks, > .specific_mapping = xgene_storm_specific_mapping, > >
On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote: > On 01/27/2014 11:34 AM, Pranavkumar Sawargaonkar wrote: > > This patch adds a reset support for xgene arm64 platform. > > > > V6: > > - Incorporating comments received on V5 patch. > > V5: > > - Incorporating comments received on V4 patch. > > V4: > > - Removing TODO comment about retriving reset base address from dts > > as that is done now. > > V3: > > - Retriving reset base address and reset mask from device tree. > > - Removed unnecssary header files included earlier. > > V2: > > - Removed unnecssary mdelay in code. > > - Adding iounmap of the base address. > > V1: > > -Initial patch. > > > > Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > > Signed-off-by: Anup Patel <anup.patel@linaro.org> > > --- > > xen/arch/arm/platforms/xgene-storm.c | 72 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 72 insertions(+) > > > > diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c > > index 5b0bd5f..4fc185b 100644 > > --- a/xen/arch/arm/platforms/xgene-storm.c > > +++ b/xen/arch/arm/platforms/xgene-storm.c > > @@ -20,8 +20,16 @@ > > > > #include <xen/config.h> > > #include <asm/platform.h> > > +#include <xen/stdbool.h> > > +#include <xen/vmap.h> > > +#include <asm/io.h> > > #include <asm/gic.h> > > > > +/* Variables to save reset address of soc during platform initialization. */ > > +static u64 reset_addr, reset_size; > > +static u32 reset_mask; > > +static bool reset_vals_valid = false; > > + > > static uint32_t xgene_storm_quirks(void) > > { > > return PLATFORM_QUIRK_GIC_64K_STRIDE; > > @@ -107,6 +115,68 @@ err: > > return ret; > > } > > > > +static void xgene_storm_reset(void) > > +{ > > I'm concerned about reset function in general, in common code we have > this code (arch/arm/shutdown.c machine_restart). > > while (1) > { > raw_machine_reset(); // which call platform_reset() > mdelay(100); > } > > If platform_reset failed, it's possible with this code, the console will > be spam with "XGENE: ...". Hrm, yes, I hadn't thought of this. > Do we really need to call raw_machine_reset multiple time? I suppose the logic is that there is no harm in trying again, we aren't doing anything else and depending on the failure it might eventually work. I think it would be reasonable to remove the print from xgene_storm_reset, or use a static int once construct. > Would it be more suitable to have this code: > > raw_machine_reset(); > mdelay(100); > printk("Failed to reset\n"); > while (1); > > Or even better, moving the mdelay per platform... I don't think that makes sense.
On 01/27/2014 02:13 PM, Ian Campbell wrote: > On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote: >> On 01/27/2014 11:34 AM, Pranavkumar Sawargaonkar wrote: >>> This patch adds a reset support for xgene arm64 platform. >>> >>> V6: >>> - Incorporating comments received on V5 patch. >>> V5: >>> - Incorporating comments received on V4 patch. >>> V4: >>> - Removing TODO comment about retriving reset base address from dts >>> as that is done now. >>> V3: >>> - Retriving reset base address and reset mask from device tree. >>> - Removed unnecssary header files included earlier. >>> V2: >>> - Removed unnecssary mdelay in code. >>> - Adding iounmap of the base address. >>> V1: >>> -Initial patch. >>> >>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>> Signed-off-by: Anup Patel <anup.patel@linaro.org> >>> --- >>> xen/arch/arm/platforms/xgene-storm.c | 72 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 72 insertions(+) >>> >>> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c >>> index 5b0bd5f..4fc185b 100644 >>> --- a/xen/arch/arm/platforms/xgene-storm.c >>> +++ b/xen/arch/arm/platforms/xgene-storm.c >>> @@ -20,8 +20,16 @@ >>> >>> #include <xen/config.h> >>> #include <asm/platform.h> >>> +#include <xen/stdbool.h> >>> +#include <xen/vmap.h> >>> +#include <asm/io.h> >>> #include <asm/gic.h> >>> >>> +/* Variables to save reset address of soc during platform initialization. */ >>> +static u64 reset_addr, reset_size; >>> +static u32 reset_mask; >>> +static bool reset_vals_valid = false; >>> + >>> static uint32_t xgene_storm_quirks(void) >>> { >>> return PLATFORM_QUIRK_GIC_64K_STRIDE; >>> @@ -107,6 +115,68 @@ err: >>> return ret; >>> } >>> >>> +static void xgene_storm_reset(void) >>> +{ >> >> I'm concerned about reset function in general, in common code we have >> this code (arch/arm/shutdown.c machine_restart). >> >> while (1) >> { >> raw_machine_reset(); // which call platform_reset() >> mdelay(100); >> } >> >> If platform_reset failed, it's possible with this code, the console will >> be spam with "XGENE: ...". > > Hrm, yes, I hadn't thought of this. > >> Do we really need to call raw_machine_reset multiple time? > > I suppose the logic is that there is no harm in trying again, we aren't > doing anything else and depending on the failure it might eventually > work. If it doesn't work the first time, why it should work the second time? I think it's platform specific to retry again if the "restart" has failed. > I think it would be reasonable to remove the print from > xgene_storm_reset, or use a static int once construct. Print are useful for debugging.
On Mon, 2014-01-27 at 14:24 +0000, Julien Grall wrote: > On 01/27/2014 02:13 PM, Ian Campbell wrote: > > On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote: > >> On 01/27/2014 11:34 AM, Pranavkumar Sawargaonkar wrote: > >>> This patch adds a reset support for xgene arm64 platform. > >>> > >>> V6: > >>> - Incorporating comments received on V5 patch. > >>> V5: > >>> - Incorporating comments received on V4 patch. > >>> V4: > >>> - Removing TODO comment about retriving reset base address from dts > >>> as that is done now. > >>> V3: > >>> - Retriving reset base address and reset mask from device tree. > >>> - Removed unnecssary header files included earlier. > >>> V2: > >>> - Removed unnecssary mdelay in code. > >>> - Adding iounmap of the base address. > >>> V1: > >>> -Initial patch. > >>> > >>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> > >>> Signed-off-by: Anup Patel <anup.patel@linaro.org> > >>> --- > >>> xen/arch/arm/platforms/xgene-storm.c | 72 ++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 72 insertions(+) > >>> > >>> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c > >>> index 5b0bd5f..4fc185b 100644 > >>> --- a/xen/arch/arm/platforms/xgene-storm.c > >>> +++ b/xen/arch/arm/platforms/xgene-storm.c > >>> @@ -20,8 +20,16 @@ > >>> > >>> #include <xen/config.h> > >>> #include <asm/platform.h> > >>> +#include <xen/stdbool.h> > >>> +#include <xen/vmap.h> > >>> +#include <asm/io.h> > >>> #include <asm/gic.h> > >>> > >>> +/* Variables to save reset address of soc during platform initialization. */ > >>> +static u64 reset_addr, reset_size; > >>> +static u32 reset_mask; > >>> +static bool reset_vals_valid = false; > >>> + > >>> static uint32_t xgene_storm_quirks(void) > >>> { > >>> return PLATFORM_QUIRK_GIC_64K_STRIDE; > >>> @@ -107,6 +115,68 @@ err: > >>> return ret; > >>> } > >>> > >>> +static void xgene_storm_reset(void) > >>> +{ > >> > >> I'm concerned about reset function in general, in common code we have > >> this code (arch/arm/shutdown.c machine_restart). > >> > >> while (1) > >> { > >> raw_machine_reset(); // which call platform_reset() > >> mdelay(100); > >> } > >> > >> If platform_reset failed, it's possible with this code, the console will > >> be spam with "XGENE: ...". > > > > Hrm, yes, I hadn't thought of this. > > > >> Do we really need to call raw_machine_reset multiple time? > > > > I suppose the logic is that there is no harm in trying again, we aren't > > doing anything else and depending on the failure it might eventually > > work. > > If it doesn't work the first time, why it should work the second time? For some platform specific reason. > I think it's platform specific to retry again if the "restart" has > failed. All that does is force every platform to reimplement the loop. > > > I think it would be reasonable to remove the print from > > xgene_storm_reset, or use a static int once construct. > > Print are useful for debugging. >
On 01/27/2014 02:27 PM, Ian Campbell wrote: > On Mon, 2014-01-27 at 14:24 +0000, Julien Grall wrote: >> On 01/27/2014 02:13 PM, Ian Campbell wrote: >>> On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote: >>>> On 01/27/2014 11:34 AM, Pranavkumar Sawargaonkar wrote: >>>>> This patch adds a reset support for xgene arm64 platform. >>>>> >>>>> V6: >>>>> - Incorporating comments received on V5 patch. >>>>> V5: >>>>> - Incorporating comments received on V4 patch. >>>>> V4: >>>>> - Removing TODO comment about retriving reset base address from dts >>>>> as that is done now. >>>>> V3: >>>>> - Retriving reset base address and reset mask from device tree. >>>>> - Removed unnecssary header files included earlier. >>>>> V2: >>>>> - Removed unnecssary mdelay in code. >>>>> - Adding iounmap of the base address. >>>>> V1: >>>>> -Initial patch. >>>>> >>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org> >>>>> Signed-off-by: Anup Patel <anup.patel@linaro.org> >>>>> --- >>>>> xen/arch/arm/platforms/xgene-storm.c | 72 ++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 72 insertions(+) >>>>> >>>>> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c >>>>> index 5b0bd5f..4fc185b 100644 >>>>> --- a/xen/arch/arm/platforms/xgene-storm.c >>>>> +++ b/xen/arch/arm/platforms/xgene-storm.c >>>>> @@ -20,8 +20,16 @@ >>>>> >>>>> #include <xen/config.h> >>>>> #include <asm/platform.h> >>>>> +#include <xen/stdbool.h> >>>>> +#include <xen/vmap.h> >>>>> +#include <asm/io.h> >>>>> #include <asm/gic.h> >>>>> >>>>> +/* Variables to save reset address of soc during platform initialization. */ >>>>> +static u64 reset_addr, reset_size; >>>>> +static u32 reset_mask; >>>>> +static bool reset_vals_valid = false; >>>>> + >>>>> static uint32_t xgene_storm_quirks(void) >>>>> { >>>>> return PLATFORM_QUIRK_GIC_64K_STRIDE; >>>>> @@ -107,6 +115,68 @@ err: >>>>> return ret; >>>>> } >>>>> >>>>> +static void xgene_storm_reset(void) >>>>> +{ >>>> >>>> I'm concerned about reset function in general, in common code we have >>>> this code (arch/arm/shutdown.c machine_restart). >>>> >>>> while (1) >>>> { >>>> raw_machine_reset(); // which call platform_reset() >>>> mdelay(100); >>>> } >>>> >>>> If platform_reset failed, it's possible with this code, the console will >>>> be spam with "XGENE: ...". >>> >>> Hrm, yes, I hadn't thought of this. >>> >>>> Do we really need to call raw_machine_reset multiple time? >>> >>> I suppose the logic is that there is no harm in trying again, we aren't >>> doing anything else and depending on the failure it might eventually >>> work. >> >> If it doesn't work the first time, why it should work the second time? > > For some platform specific reason. > >> I think it's platform specific to retry again if the "restart" has >> failed. > > All that does is force every platform to reimplement the loop. Not every platform. For instance on the Arndale and the Versatile Express we don't need the loop. After looking to the reset code of X-Gene on Linux it's the same. I'm surprised that we would need the loop in Xen. The only ways we can fail are: - ioremap return NULL; - reset address is not set. Both won't work at the second attempd, neither the third.
On Mon, 2014-01-27 at 14:36 +0000, Julien Grall wrote: > The only ways we can fail are: > - ioremap return NULL; > - reset address is not set. > > Both won't work at the second attempd, neither the third. You are missing the fact that the write to the reset address itself may "succeed" but not do anything, for example because of firmware oddities or some strange platform specific property. It might succeed on a second or third attempt, and there is no harm in trying. Ian.
On Mon, 2014-01-27 at 12:07 +0000, George Dunlap wrote: > Yes, "can't reboot via software" is a pretty big lack of functionality; > it weighs in pretty heavily against whatever potential regressions might > be introduced. > > Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com> I've applied this. I think Julien's concerns about the boot loop spamming the consoleif reboot fails are 4.5 material. Ian.
On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote:
> + DT_MATCH_COMPATIBLE("apm,xgene-reboot"),
I should have asked this sooner -- can you point me to the bindings
documentation for this device?
http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it
is not yet agreed, so having Xen depend on it may have been a mistake.
Ian.
Hi Ian, On 28 January 2014 21:17, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote: >> + DT_MATCH_COMPATIBLE("apm,xgene-reboot"), > > I should have asked this sooner -- can you point me to the bindings > documentation for this device? > > http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it > is not yet agreed, so having Xen depend on it may have been a mistake. Above patch is still under discussion so i can not take changes from that to xen driver immediately. For now i have added xen reset code based on "drivers/power/reset/xgene-reboot.c" driver which is already merged in linux. http://www.spinics.net/lists/arm-kernel/msg266039.html For now DTS bindings for xen are similar as mentioned in above link. Actually if you see new patch and old one (from reboot point of view) - Only difference in both the dts bindings is "mask" filed in dts. In old patch it used to be read from dts but in latest it is hard-coded to 1 in actual code and being removed from dts in new patch. Now if you want this to be fixed , i can quickly submit a V7 in which mask field will be just hard-coded to 1 hence xen code will always work even if linux code does gets changed. Thanks, Pranav > > Ian. >
On Tue, 2014-01-28 at 23:05 +0530, Pranavkumar Sawargaonkar wrote: > Hi Ian, > > On 28 January 2014 21:17, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote: > >> + DT_MATCH_COMPATIBLE("apm,xgene-reboot"), > > > > I should have asked this sooner -- can you point me to the bindings > > documentation for this device? > > > > http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it > > is not yet agreed, so having Xen depend on it may have been a mistake. > > Above patch is still under discussion so i can not take changes from > that to xen driver immediately. > > For now i have added xen reset code based on > "drivers/power/reset/xgene-reboot.c" driver which is already merged in > linux. > > http://www.spinics.net/lists/arm-kernel/msg266039.html > For now DTS bindings for xen are similar as mentioned in above link. > > Actually if you see new patch and old one (from reboot point of view) - > Only difference in both the dts bindings is "mask" filed in dts. > In old patch it used to be read from dts but in latest it is > hard-coded to 1 in actual code and being removed from dts in new > patch. Do you have a ref for that new patch? I also don't see any patch to linux/Documentation/devicetree/bindings, as was requested in that posting from 6 months ago. Where can I find that? It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also hasn't landed? > Now if you want this to be fixed , i can quickly submit a V7 in which > mask field will be just hard-coded to 1 hence xen code will always > work even if linux code does gets changed. Looks like the Linux driver uses 0xffffffff if the mask isn't given -- that seems like a good approach. I think we'll just have to accept that until the binding is specified and documented (in linux/Documentation/devicetree/bindings) then we may have to be prepared to change the Xen implementation to match the final spec without regard to backwards compat. If we aren't happy with that then I should revert the patch now and we will have to live without reboot support in the meantime. Ian
Hi Ian, On 28 January 2014 23:13, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Tue, 2014-01-28 at 23:05 +0530, Pranavkumar Sawargaonkar wrote: >> Hi Ian, >> >> On 28 January 2014 21:17, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote: >> >> + DT_MATCH_COMPATIBLE("apm,xgene-reboot"), >> > >> > I should have asked this sooner -- can you point me to the bindings >> > documentation for this device? >> > >> > http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it >> > is not yet agreed, so having Xen depend on it may have been a mistake. >> >> Above patch is still under discussion so i can not take changes from >> that to xen driver immediately. >> >> For now i have added xen reset code based on >> "drivers/power/reset/xgene-reboot.c" driver which is already merged in >> linux. >> >> http://www.spinics.net/lists/arm-kernel/msg266039.html >> For now DTS bindings for xen are similar as mentioned in above link. >> >> Actually if you see new patch and old one (from reboot point of view) - >> Only difference in both the dts bindings is "mask" filed in dts. >> In old patch it used to be read from dts but in latest it is >> hard-coded to 1 in actual code and being removed from dts in new >> patch. > > Do you have a ref for that new patch? New patch is the one you have mentioned i.e. http://www.gossamer-threads.com/lists/linux/kernel/1845585 It has new DT bindings. > > I also don't see any patch to linux/Documentation/devicetree/bindings, > as was requested in that posting from 6 months ago. Where can I find > that? > > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also > hasn't landed? Yeah it is dangling and since new patch is already posted i think we can wait for final DT bindings. > >> Now if you want this to be fixed , i can quickly submit a V7 in which >> mask field will be just hard-coded to 1 hence xen code will always >> work even if linux code does gets changed. > > Looks like the Linux driver uses 0xffffffff if the mask isn't given -- > that seems like a good approach. > > I think we'll just have to accept that until the binding is specified > and documented (in linux/Documentation/devicetree/bindings) then we may > have to be prepared to change the Xen implementation to match the final > spec without regard to backwards compat. If we aren't happy with that > then I should revert the patch now and we will have to live without > reboot support in the meantime. Please do not revert the patch , I think we can go ahead with current patch. Once linux side is concluded i will fix minor changes in xen code based on new DT bindigs.. > > Ian > Thanks, Pranav
On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote: > > I also don't see any patch to linux/Documentation/devicetree/bindings, > > as was requested in that posting from 6 months ago. Where can I find > > that? > > > > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also > > hasn't landed? > Yeah it is dangling and since new patch is already posted i think we > can wait for final DT bindings. It seems from the thread that the final bindings are going to differ significantly from what is implemented in Xen and proposed in the above thread. (with a syscon driver that the reset driver references). > >> Now if you want this to be fixed , i can quickly submit a V7 in which > >> mask field will be just hard-coded to 1 hence xen code will always > >> work even if linux code does gets changed. > > > > Looks like the Linux driver uses 0xffffffff if the mask isn't given -- > > that seems like a good approach. > > > > I think we'll just have to accept that until the binding is specified > > and documented (in linux/Documentation/devicetree/bindings) then we may > > have to be prepared to change the Xen implementation to match the final > > spec without regard to backwards compat. If we aren't happy with that > > then I should revert the patch now and we will have to live without > > reboot support in the meantime. > Please do not revert the patch , I think we can go ahead with current patch. > Once linux side is concluded i will fix minor changes in xen code > based on new DT bindigs.. It doesn't sounds to me like it is going to be minor changes. Ian.
Hi Ian, On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote: > >> > I also don't see any patch to linux/Documentation/devicetree/bindings, >> > as was requested in that posting from 6 months ago. Where can I find >> > that? >> > >> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also >> > hasn't landed? >> Yeah it is dangling and since new patch is already posted i think we >> can wait for final DT bindings. > > It seems from the thread that the final bindings are going to differ > significantly from what is implemented in Xen and proposed in the above > thread. (with a syscon driver that the reset driver references). > >> >> Now if you want this to be fixed , i can quickly submit a V7 in which >> >> mask field will be just hard-coded to 1 hence xen code will always >> >> work even if linux code does gets changed. >> > >> > Looks like the Linux driver uses 0xffffffff if the mask isn't given -- >> > that seems like a good approach. >> > >> > I think we'll just have to accept that until the binding is specified >> > and documented (in linux/Documentation/devicetree/bindings) then we may >> > have to be prepared to change the Xen implementation to match the final >> > spec without regard to backwards compat. If we aren't happy with that >> > then I should revert the patch now and we will have to live without >> > reboot support in the meantime. >> Please do not revert the patch , I think we can go ahead with current patch. >> Once linux side is concluded i will fix minor changes in xen code >> based on new DT bindigs.. > > It doesn't sounds to me like it is going to be minor changes. Yes binding are changed in new drivers but now question is what to do in current state where new driver is not submitted ? My take is we have 3 opts : 1. Keep current reboot driver in xen as it is and use it with old bindings. (since that is the one merged in linux) 2. I will send a new patch (will take 1hr max for me to do it) with addresses hardcoded instead of reading it from dts. This will help for xen to have reboot driver for xgene. 3. Remove this driver completely from xen as of now. > > Ian. > Thanks, Pranav
On Thu, 2014-01-30 at 11:38 +0530, Pranavkumar Sawargaonkar wrote: > Hi Ian, > > On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote: > > > >> > I also don't see any patch to linux/Documentation/devicetree/bindings, > >> > as was requested in that posting from 6 months ago. Where can I find > >> > that? > >> > > >> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also > >> > hasn't landed? > >> Yeah it is dangling and since new patch is already posted i think we > >> can wait for final DT bindings. > > > > It seems from the thread that the final bindings are going to differ > > significantly from what is implemented in Xen and proposed in the above > > thread. (with a syscon driver that the reset driver references). > > > >> >> Now if you want this to be fixed , i can quickly submit a V7 in which > >> >> mask field will be just hard-coded to 1 hence xen code will always > >> >> work even if linux code does gets changed. > >> > > >> > Looks like the Linux driver uses 0xffffffff if the mask isn't given -- > >> > that seems like a good approach. > >> > > >> > I think we'll just have to accept that until the binding is specified > >> > and documented (in linux/Documentation/devicetree/bindings) then we may > >> > have to be prepared to change the Xen implementation to match the final > >> > spec without regard to backwards compat. If we aren't happy with that > >> > then I should revert the patch now and we will have to live without > >> > reboot support in the meantime. > >> Please do not revert the patch , I think we can go ahead with current patch. > >> Once linux side is concluded i will fix minor changes in xen code > >> based on new DT bindigs.. > > > > It doesn't sounds to me like it is going to be minor changes. > Yes binding are changed in new drivers but now question is what to do > in current state where new driver is not submitted ? > > My take is we have 3 opts : > 1. Keep current reboot driver in xen as it is and use it with old > bindings. (since that is the one merged in linux) > 2. I will send a new patch (will take 1hr max for me to do it) with > addresses hardcoded instead of reading it from dts. > This will help for xen to have reboot driver for xgene. > 3. Remove this driver completely from xen as of now. None of the options are brilliant :-/ I think on balance #2 is probably the way to go. #1 would set a precedent for using formally undefined bindings which I think we should avoid. #3 has obvious downsides, but given that we have already accepted the functionality it seems a shame to revert it entirely. Ian.
Hi Ian, On 3 February 2014 22:34, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Thu, 2014-01-30 at 11:38 +0530, Pranavkumar Sawargaonkar wrote: >> Hi Ian, >> >> On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote: >> > >> >> > I also don't see any patch to linux/Documentation/devicetree/bindings, >> >> > as was requested in that posting from 6 months ago. Where can I find >> >> > that? >> >> > >> >> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also >> >> > hasn't landed? >> >> Yeah it is dangling and since new patch is already posted i think we >> >> can wait for final DT bindings. >> > >> > It seems from the thread that the final bindings are going to differ >> > significantly from what is implemented in Xen and proposed in the above >> > thread. (with a syscon driver that the reset driver references). >> > >> >> >> Now if you want this to be fixed , i can quickly submit a V7 in which >> >> >> mask field will be just hard-coded to 1 hence xen code will always >> >> >> work even if linux code does gets changed. >> >> > >> >> > Looks like the Linux driver uses 0xffffffff if the mask isn't given -- >> >> > that seems like a good approach. >> >> > >> >> > I think we'll just have to accept that until the binding is specified >> >> > and documented (in linux/Documentation/devicetree/bindings) then we may >> >> > have to be prepared to change the Xen implementation to match the final >> >> > spec without regard to backwards compat. If we aren't happy with that >> >> > then I should revert the patch now and we will have to live without >> >> > reboot support in the meantime. >> >> Please do not revert the patch , I think we can go ahead with current patch. >> >> Once linux side is concluded i will fix minor changes in xen code >> >> based on new DT bindigs.. >> > >> > It doesn't sounds to me like it is going to be minor changes. >> Yes binding are changed in new drivers but now question is what to do >> in current state where new driver is not submitted ? >> >> My take is we have 3 opts : >> 1. Keep current reboot driver in xen as it is and use it with old >> bindings. (since that is the one merged in linux) >> 2. I will send a new patch (will take 1hr max for me to do it) with >> addresses hardcoded instead of reading it from dts. >> This will help for xen to have reboot driver for xgene. >> 3. Remove this driver completely from xen as of now. > > None of the options are brilliant :-/ > > I think on balance #2 is probably the way to go. Even i think this is the best way among all these patchy options :P Tomorrow I will send a new patch with removal of dts stuff from xen driver so that it will always work irrespective of linux stuff. Once linux side comes to conclusion and merged i will reintroduce dts stuff in this driver. > > #1 would set a precedent for using formally undefined bindings which I > think we should avoid. > > #3 has obvious downsides, but given that we have already accepted the > functionality it seems a shame to revert it entirely. > > Ian. > - Pranav
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index 5b0bd5f..4fc185b 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -20,8 +20,16 @@ #include <xen/config.h> #include <asm/platform.h> +#include <xen/stdbool.h> +#include <xen/vmap.h> +#include <asm/io.h> #include <asm/gic.h> +/* Variables to save reset address of soc during platform initialization. */ +static u64 reset_addr, reset_size; +static u32 reset_mask; +static bool reset_vals_valid = false; + static uint32_t xgene_storm_quirks(void) { return PLATFORM_QUIRK_GIC_64K_STRIDE; @@ -107,6 +115,68 @@ err: return ret; } +static void xgene_storm_reset(void) +{ + void __iomem *addr; + + if ( !reset_vals_valid ) + { + printk("XGENE: Invalid reset values, can not reset XGENE...\n"); + return; + } + + addr = ioremap_nocache(reset_addr, reset_size); + + if ( !addr ) + { + printk("XGENE: Unable to map xgene reset address, can not reset XGENE...\n"); + return; + } + + /* Write reset mask to base address */ + writel(reset_mask, addr); + + iounmap(addr); +} + +static int xgene_storm_init(void) +{ + static const struct dt_device_match reset_ids[] __initconst = + { + DT_MATCH_COMPATIBLE("apm,xgene-reboot"), + {}, + }; + struct dt_device_node *dev; + int res; + + dev = dt_find_matching_node(NULL, reset_ids); + if ( !dev ) + { + printk("XGENE: Unable to find a compatible reset node in the device tree"); + return 0; + } + + dt_device_set_used_by(dev, DOMID_XEN); + + /* Retrieve base address and size */ + res = dt_device_get_address(dev, 0, &reset_addr, &reset_size); + if ( res ) + { + printk("XGENE: Unable to retrieve the base address for reset\n"); + return 0; + } + + /* Get reset mask */ + res = dt_property_read_u32(dev, "mask", &reset_mask); + if ( !res ) + { + printk("XGENE: Unable to retrieve the reset mask\n"); + return 0; + } + + reset_vals_valid = true; + return 0; +} static const char * const xgene_storm_dt_compat[] __initconst = { @@ -116,6 +186,8 @@ static const char * const xgene_storm_dt_compat[] __initconst = PLATFORM_START(xgene_storm, "APM X-GENE STORM") .compatible = xgene_storm_dt_compat, + .init = xgene_storm_init, + .reset = xgene_storm_reset, .quirks = xgene_storm_quirks, .specific_mapping = xgene_storm_specific_mapping,