Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Philipp, On 10/8/2019 11:56 PM, Philipp Zabel wrote: Hi Martin, On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote: Hi Philipp, On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: [...] because the register layout was greatly simplified for the newer SoCs (for which there is reset-intel) compared to the older ones (reset-lantiq). Dilip's suggestion (in my own words) is that you take his new reset-intel driver, then we will work on porting reset-lantiq over to that so in the end we can drop the reset-lantiq driver. Just to be sure, you are suggesting to add support for the current lantiq,reset binding to the reset-intel driver at a later point? I see no reason not to do that, but I'm also not quite sure what the benefit will be over just keeping reset-lantiq as is? according to Chuan and Dilip the current reset-lantiq implementation is wrong [0]. The only issue seems to be the .reset callback, which doesn't have any users anway. The DT binding of reset-lantiq driver is also having issue. I have explained here [1]. my understanding is that the Lantiq and Intel LGM reset controllers are identical except: - the Lantiq variant uses a weird register layout (reset and status registers not at consecutive offsets) - the bits of the reset and status registers sometimes don't match on the Lantiq variant Thank you, so these are a good explanation for why the DT bindings should be different. - the Intel variant has a dedicated registers area for the reset controller registers, while the Lantiq variant mixes them with various other functionality (for example: USB2 PHYs) I'm not quite sure I understand why the intel driver is using syscon, then. Either way, it shouldn't make a big difference if regmap is used anyway. Yes, we decided to remove the syscon and use the regmap.[2] This approach means more work for me (as I am probably the one who then has to do the work to port reset-lantiq over to reset-intel). More work than what alternative? compared to "fixing" the existing reset-lantiq driver (reset callback) That is still something you could do, or just drop the .reset callback because there are no reset consumers using it anyway. One correct thing to do would be to identify those self-clearing reset bits and to disallow calling assert/deassert on them. and then (instead of adding a new driver) integrating Intel LGM support into reset-lantiq Since at this point I'm not even sure whether merging the two at all is better than keeping them separate, I have no opinion on whether merging intel support into the lantiq driver or the other way around is preferable. I'm happy to do that work if you think that it's worth following this approach. So I want your opinion on this before I spend any effort on porting reset-lantiq over to reset-intel. Reset drivers are typically so simple, I'm not quite sure whether it is worth to integrate multiple drivers if it complicates matters too much. In this case though I expect it would just be adding support for a custom .of_xlate and lantiq specific register property parsing? yes, that's how I understand the Lantiq and Intel reset controllers: - reset/status/assert/deassert callbacks would be shared across all variants - register parsing and of_xlate are SoC specific Ok. If that turns out to be less rather than more boilerplate than two separate drivers, that should be fine. Thanks Philipp for your time and briefly explaining your view. Regards, Dilip [1]: https://www.spinics.net/lists/devicetree/msg308930.html [2]: https://lkml.org/lkml/2019/9/2/289 regards Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote: > Hi Philipp, > > On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: > [...] > > > because the register layout was greatly simplified for the newer SoCs > > > (for which there is reset-intel) compared to the older ones > > > (reset-lantiq). > > > Dilip's suggestion (in my own words) is that you take his new > > > reset-intel driver, then we will work on porting reset-lantiq over to > > > that so in the end we can drop the reset-lantiq driver. > > > > Just to be sure, you are suggesting to add support for the current > > lantiq,reset binding to the reset-intel driver at a later point? I > > see no reason not to do that, but I'm also not quite sure what the > > benefit will be over just keeping reset-lantiq as is? > > according to Chuan and Dilip the current reset-lantiq implementation > is wrong [0]. The only issue seems to be the .reset callback, which doesn't have any users anway. > my understanding is that the Lantiq and Intel LGM reset controllers > are identical except: > - the Lantiq variant uses a weird register layout (reset and status > registers not at consecutive offsets) > - the bits of the reset and status registers sometimes don't match on > the Lantiq variant Thank you, so these are a good explanation for why the DT bindings should be different. > - the Intel variant has a dedicated registers area for the reset > controller registers, while the Lantiq variant mixes them with various > other functionality (for example: USB2 PHYs) I'm not quite sure I understand why the intel driver is using syscon, then. Either way, it shouldn't make a big difference if regmap is used anyway. > > > This approach means more work for me (as I am probably the one who > > > then has to do the work to port reset-lantiq over to reset-intel). > > > > More work than what alternative? > > compared to "fixing" the existing reset-lantiq driver (reset callback) That is still something you could do, or just drop the .reset callback because there are no reset consumers using it anyway. One correct thing to do would be to identify those self-clearing reset bits and to disallow calling assert/deassert on them. > and then (instead of adding a new driver) integrating Intel LGM > support into reset-lantiq Since at this point I'm not even sure whether merging the two at all is better than keeping them separate, I have no opinion on whether merging intel support into the lantiq driver or the other way around is preferable. > > > I'm happy to do that work if you think that it's worth following this > > > approach. So I want your opinion on this before I spend any effort on > > > porting reset-lantiq over to reset-intel. > > > > Reset drivers are typically so simple, I'm not quite sure whether it is > > worth to integrate multiple drivers if it complicates matters too much. > > In this case though I expect it would just be adding support for a > > custom .of_xlate and lantiq specific register property parsing? > > yes, that's how I understand the Lantiq and Intel reset controllers: > - reset/status/assert/deassert callbacks would be shared across all variants > - register parsing and of_xlate are SoC specific Ok. If that turns out to be less rather than more boilerplate than two separate drivers, that should be fine. regards Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,Philipp, On 10/8/2019 3:53 AM, Martin Blumenstingl wrote: Hi Philipp, On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: [...] because the register layout was greatly simplified for the newer SoCs (for which there is reset-intel) compared to the older ones (reset-lantiq). Dilip's suggestion (in my own words) is that you take his new reset-intel driver, then we will work on porting reset-lantiq over to that so in the end we can drop the reset-lantiq driver. Just to be sure, you are suggesting to add support for the current lantiq,reset binding to the reset-intel driver at a later point? I see no reason not to do that, but I'm also not quite sure what the benefit will be over just keeping reset-lantiq as is? according to Chuan and Dilip the current reset-lantiq implementation is wrong [0]. my understanding is that the Lantiq and Intel LGM reset controllers are identical except: - the Lantiq variant uses a weird register layout (reset and status registers not at consecutive offsets) - the bits of the reset and status registers sometimes don't match on the Lantiq variant - the Intel variant has a dedicated registers area for the reset controller registers, while the Lantiq variant mixes them with various other functionality (for example: USB2 PHYs) This approach means more work for me (as I am probably the one who then has to do the work to port reset-lantiq over to reset-intel). More work than what alternative? compared to "fixing" the existing reset-lantiq driver (reset callback) and then (instead of adding a new driver) integrating Intel LGM support into reset-lantiq Integrating Intel LGM support into reset-lantiq boils down to re-writing reset-lantiq driver as intel-reset driver and adding Lantiq variant support. Why because reset-lantiq driver is not according to hardware design[1]. I see the final best solution is to integrate Lantiq variant driver to intel-reset driver.[1] I hope you guys are ok with it. Please let me know your view. Regards, Dilip [1]: https://www.spinics.net/lists/devicetree/msg308930.html I'm happy to do that work if you think that it's worth following this approach. So I want your opinion on this before I spend any effort on porting reset-lantiq over to reset-intel. Reset drivers are typically so simple, I'm not quite sure whether it is worth to integrate multiple drivers if it complicates matters too much. In this case though I expect it would just be adding support for a custom .of_xlate and lantiq specific register property parsing? yes, that's how I understand the Lantiq and Intel reset controllers: - reset/status/assert/deassert callbacks would be shared across all variants - register parsing and of_xlate are SoC specific Martin [0] https://www.spinics.net/lists/devicetree/msg305951.html
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Philipp, On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: [...] > > because the register layout was greatly simplified for the newer SoCs > > (for which there is reset-intel) compared to the older ones > > (reset-lantiq). > > Dilip's suggestion (in my own words) is that you take his new > > reset-intel driver, then we will work on porting reset-lantiq over to > > that so in the end we can drop the reset-lantiq driver. > > Just to be sure, you are suggesting to add support for the current > lantiq,reset binding to the reset-intel driver at a later point? I > see no reason not to do that, but I'm also not quite sure what the > benefit will be over just keeping reset-lantiq as is? according to Chuan and Dilip the current reset-lantiq implementation is wrong [0]. my understanding is that the Lantiq and Intel LGM reset controllers are identical except: - the Lantiq variant uses a weird register layout (reset and status registers not at consecutive offsets) - the bits of the reset and status registers sometimes don't match on the Lantiq variant - the Intel variant has a dedicated registers area for the reset controller registers, while the Lantiq variant mixes them with various other functionality (for example: USB2 PHYs) > > This approach means more work for me (as I am probably the one who > > then has to do the work to port reset-lantiq over to reset-intel). > > More work than what alternative? compared to "fixing" the existing reset-lantiq driver (reset callback) and then (instead of adding a new driver) integrating Intel LGM support into reset-lantiq > > I'm happy to do that work if you think that it's worth following this > > approach. So I want your opinion on this before I spend any effort on > > porting reset-lantiq over to reset-intel. > > Reset drivers are typically so simple, I'm not quite sure whether it is > worth to integrate multiple drivers if it complicates matters too much. > In this case though I expect it would just be adding support for a > custom .of_xlate and lantiq specific register property parsing? yes, that's how I understand the Lantiq and Intel reset controllers: - reset/status/assert/deassert callbacks would be shared across all variants - register parsing and of_xlate are SoC specific Martin [0] https://www.spinics.net/lists/devicetree/msg305951.html
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, Dilip,
On Thu, Sep 19, 2019 at 09:51:48PM +0200, Martin Blumenstingl wrote:
> Hi Dilip,
>
> (sorry for the late reply)
Same, sorry for the delay.
> On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota
> wrote:
> [...]
> > The major difference between the vrx200 and lgm is:
> > 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
> > has one single register region.
> > 2.) Register offsets and bit offsets are different.
> >
> > So enhancing the intel-reset-syscon.c to provide compatibility/support
> > for vrx200.
> > Please check the below dtsi binding proposal and let me know your view.
> >
> > rcu0:reset-controller@ {
> > compatible= "intel,rcu-lgm";
> > reg = <0x000 0x8>, , ,
> > ;
> I'm not sure that I understand what are reg_set2/3/4 for
> the first resource (0x8 at 0x0) already covers the whole LGM RCU,
> so what is the purpose of the other register resources
>
> > intel,global-reset = <0x10 30>;
> > #reset-cells = <3>;
> > };
> >
> > "#reset-cells":
> >const:3
> >description: |
> > The 1st cell is the reset register offset.
> > The 2nd cell is the reset set bit offset.
> > The 3rd cell is the reset status bit offset.
> I think this will work fine for VRX200 (and even older SoCs)
> as you have described in your previous emails we can determine the
> status offset from the reset offset using a simple if/else
>
> for LGM I like your initial suggestion with #reset-cells = <2> because
> it's easier to read and write.
>
> > Reset driver takes care of parsing the register address "reg" as per the
> > ".data" structure in struct of_device_id.
> > Reset driver takes care of traversing the status register offset.
> the differentiation between two and three #reset-cells can also happen
> based on the struct of_device_id:
> - the LGM implementation would simply also use the reset bit as status
> bit (only two cells are needed)
> - the implementation for earlier SoCs would parse the third cell and
> use that as status bit
>
> Philipp, can you please share your opinion on how to move forward with
> the reset-intel driver from this series?
That all sounds reasonable for VRX200/LGM to me.
> because the register layout was greatly simplified for the newer SoCs
> (for which there is reset-intel) compared to the older ones
> (reset-lantiq).
> Dilip's suggestion (in my own words) is that you take his new
> reset-intel driver, then we will work on porting reset-lantiq over to
> that so in the end we can drop the reset-lantiq driver.
Just to be sure, you are suggesting to add support for the current
lantiq,reset binding to the reset-intel driver at a later point? I
see no reason not to do that, but I'm also not quite sure what the
benefit will be over just keeping reset-lantiq as is?
> This approach means more work for me (as I am probably the one who
> then has to do the work to port reset-lantiq over to reset-intel).
More work than what alternative?
> I'm happy to do that work if you think that it's worth following this
> approach. So I want your opinion on this before I spend any effort on
> porting reset-lantiq over to reset-intel.
Reset drivers are typically so simple, I'm not quite sure whether it is
worth to integrate multiple drivers if it complicates matters too much.
In this case though I expect it would just be adding support for a
custom .of_xlate and lantiq specific register property parsing?
regards
Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin and Philipp,
On 20/9/2019 10:47 AM, Dilip Kota wrote:
Hi Martin,
On 9/20/2019 3:51 AM, Martin Blumenstingl wrote:
Hi Dilip,
(sorry for the late reply)
On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota
wrote:
[...]
The major difference between the vrx200 and lgm is:
1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
has one single register region.
2.) Register offsets and bit offsets are different.
So enhancing the intel-reset-syscon.c to provide compatibility/support
for vrx200.
Please check the below dtsi binding proposal and let me know your view.
rcu0:reset-controller@ {
compatible= "intel,rcu-lgm";
reg = <0x000 0x8>, , ,
;
I'm not sure that I understand what are reg_set2/3/4 for
the first resource (0x8 at 0x0) already covers the whole LGM RCU,
so what is the purpose of the other register resources
Yes, as you said the first register resource is enough for LGM RCU as
registers are at one single region. Whereas in older SoCs RCU
registers are at different regions, so for that reason reg_set2/3/4
are used.
Driver will decide in reading the no. of register resources based on
the "struct of_device_id".
Regards,
Dilip
intel,global-reset = <0x10 30>;
#reset-cells = <3>;
};
"#reset-cells":
const:3
description: |
The 1st cell is the reset register offset.
The 2nd cell is the reset set bit offset.
The 3rd cell is the reset status bit offset.
I think this will work fine for VRX200 (and even older SoCs)
as you have described in your previous emails we can determine the
status offset from the reset offset using a simple if/else
for LGM I like your initial suggestion with #reset-cells = <2> because
it's easier to read and write.
Reset driver takes care of parsing the register address "reg" as per the
".data" structure in struct of_device_id.
Reset driver takes care of traversing the status register offset.
the differentiation between two and three #reset-cells can also happen
based on the struct of_device_id:
- the LGM implementation would simply also use the reset bit as status
bit (only two cells are needed)
- the implementation for earlier SoCs would parse the third cell and
use that as status bit
Philipp, can you please share your opinion on how to move forward with
the reset-intel driver from this series?
The reset_control_ops from the reset-intel driver are (in my opinion)
a bug-fixed and improved version of what we already have in
drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
because the register layout was greatly simplified for the newer SoCs
(for which there is reset-intel) compared to the older ones
(reset-lantiq).
Dilip's suggestion (in my own words) is that you take his new
reset-intel driver, then we will work on porting reset-lantiq over to
that so in the end we can drop the reset-lantiq driver. This approach
means more work for me (as I am probably the one who then has to do
the work to port reset-lantiq over to reset-intel). I'm happy to do
that work if you think that it's worth following this approach.
So I want your opinion on this before I spend any effort on porting
reset-lantiq over to reset-intel.
I will start implementing this design in the next patch version along
with the other changes suggested in this patch review, please let me
know if you have other thoughts in this design.
Regards,
Dilip
Martin
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 9/20/2019 3:51 AM, Martin Blumenstingl wrote:
Hi Dilip,
(sorry for the late reply)
On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota wrote:
[...]
The major difference between the vrx200 and lgm is:
1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
has one single register region.
2.) Register offsets and bit offsets are different.
So enhancing the intel-reset-syscon.c to provide compatibility/support
for vrx200.
Please check the below dtsi binding proposal and let me know your view.
rcu0:reset-controller@ {
compatible= "intel,rcu-lgm";
reg = <0x000 0x8>, , ,
;
I'm not sure that I understand what are reg_set2/3/4 for
the first resource (0x8 at 0x0) already covers the whole LGM RCU,
so what is the purpose of the other register resources
Yes, as you said the first register resource is enough for LGM RCU as
registers are at one single region. Whereas in older SoCs RCU registers
are at different regions, so for that reason reg_set2/3/4 are used.
Driver will decide in reading the no. of register resources based on the
"struct of_device_id".
Regards,
Dilip
intel,global-reset = <0x10 30>;
#reset-cells = <3>;
};
"#reset-cells":
const:3
description: |
The 1st cell is the reset register offset.
The 2nd cell is the reset set bit offset.
The 3rd cell is the reset status bit offset.
I think this will work fine for VRX200 (and even older SoCs)
as you have described in your previous emails we can determine the
status offset from the reset offset using a simple if/else
for LGM I like your initial suggestion with #reset-cells = <2> because
it's easier to read and write.
Reset driver takes care of parsing the register address "reg" as per the
".data" structure in struct of_device_id.
Reset driver takes care of traversing the status register offset.
the differentiation between two and three #reset-cells can also happen
based on the struct of_device_id:
- the LGM implementation would simply also use the reset bit as status
bit (only two cells are needed)
- the implementation for earlier SoCs would parse the third cell and
use that as status bit
Philipp, can you please share your opinion on how to move forward with
the reset-intel driver from this series?
The reset_control_ops from the reset-intel driver are (in my opinion)
a bug-fixed and improved version of what we already have in
drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
because the register layout was greatly simplified for the newer SoCs
(for which there is reset-intel) compared to the older ones
(reset-lantiq).
Dilip's suggestion (in my own words) is that you take his new
reset-intel driver, then we will work on porting reset-lantiq over to
that so in the end we can drop the reset-lantiq driver. This approach
means more work for me (as I am probably the one who then has to do
the work to port reset-lantiq over to reset-intel). I'm happy to do
that work if you think that it's worth following this approach.
So I want your opinion on this before I spend any effort on porting
reset-lantiq over to reset-intel.
Martin
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Dilip,
(sorry for the late reply)
On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota wrote:
[...]
> The major difference between the vrx200 and lgm is:
> 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
> has one single register region.
> 2.) Register offsets and bit offsets are different.
>
> So enhancing the intel-reset-syscon.c to provide compatibility/support
> for vrx200.
> Please check the below dtsi binding proposal and let me know your view.
>
> rcu0:reset-controller@ {
> compatible= "intel,rcu-lgm";
> reg = <0x000 0x8>, , ,
> ;
I'm not sure that I understand what are reg_set2/3/4 for
the first resource (0x8 at 0x0) already covers the whole LGM RCU,
so what is the purpose of the other register resources
> intel,global-reset = <0x10 30>;
> #reset-cells = <3>;
> };
>
> "#reset-cells":
>const:3
>description: |
> The 1st cell is the reset register offset.
> The 2nd cell is the reset set bit offset.
> The 3rd cell is the reset status bit offset.
I think this will work fine for VRX200 (and even older SoCs)
as you have described in your previous emails we can determine the
status offset from the reset offset using a simple if/else
for LGM I like your initial suggestion with #reset-cells = <2> because
it's easier to read and write.
> Reset driver takes care of parsing the register address "reg" as per the
> ".data" structure in struct of_device_id.
> Reset driver takes care of traversing the status register offset.
the differentiation between two and three #reset-cells can also happen
based on the struct of_device_id:
- the LGM implementation would simply also use the reset bit as status
bit (only two cells are needed)
- the implementation for earlier SoCs would parse the third cell and
use that as status bit
Philipp, can you please share your opinion on how to move forward with
the reset-intel driver from this series?
The reset_control_ops from the reset-intel driver are (in my opinion)
a bug-fixed and improved version of what we already have in
drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
because the register layout was greatly simplified for the newer SoCs
(for which there is reset-intel) compared to the older ones
(reset-lantiq).
Dilip's suggestion (in my own words) is that you take his new
reset-intel driver, then we will work on porting reset-lantiq over to
that so in the end we can drop the reset-lantiq driver. This approach
means more work for me (as I am probably the one who then has to do
the work to port reset-lantiq over to reset-intel). I'm happy to do
that work if you think that it's worth following this approach.
So I want your opinion on this before I spend any effort on porting
reset-lantiq over to reset-intel.
Martin
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Thomas, On 9/19/2019 4:36 PM, Langer, Thomas wrote: Hi Dilip, -Original Message- From: devicetree-ow...@vger.kernel.org On Behalf Of Dilip Kota Sent: Donnerstag, 19. September 2019 10:06 To: Martin Blumenstingl Cc: Chuan Hua, Lei ; Kim, Cheol Yong ; devicet...@vger.kernel.org; linux- ker...@vger.kernel.org; p.za...@pengutronix.de; Wu, Qiming ; r...@kernel.org; Hauke Mehrtens Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC Hi Martin, On 9/12/2019 2:38 PM, Dilip Kota wrote: Re-sending the mail, because of delivery failure. sorry for the spam. Hi Martin, On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: Hi, On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei wrote: [...] I'm not surprised that we got some of the IP block layout for the VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW (BSP). with proper documentation (as in a "public datasheet for the SoC") it would be easy to spot these mistakes (at least I assume that the quality of the Infineon / Lantiq datasheets is excellent). back to reset-intel-syscon: assigning only one job to the RCU hardware is a good idea (in my opinion). that brings up a question: why do we need the "syscon" compatible for the RCU node? this is typically used when registers are accessed by another IP block and the other driver has to access these registers as well. does this mean that there's more hidden in the RCU registers? As I mentioned, some other misc registers are put into RCU even they don't belong to reset functions. OK, just be aware that there are also rules for syscon compatible drivers, see for example: [0] if Rob (dt-bindings maintainer) is happy with the documentation in patch 1 then I'm fine with it as well. for my own education I would appreciate if you could describe these "other misc registers" with a few sentences (I assume that this can also help Rob) For LGM, RCU is clean. There would be no MISC register after software's feedback. These misc registers will be moved to chiptop/misc groups(implemented by syscon). For legacy SoC, we do have a lot MISC registers for different SoCs. OK, I think I understand now: chiptop != RCU so RCU really only has one purpose: handling resets while chiptop manages all the random bits does this means we don't need RCU to match "syscon"? If we don't support legacy SoC with the same driver, we don't need syscon, just regmap. Regmap is a must for us since we will use regmap proxy to implement secure rest via secure processor. I think we should drop the syscon compatible for LGM then even for the legacy SoCs the reset controller should not have a syscon compatible: instead it should have a syscon parent (as the current "lantiq,xrx200-reset" binding requires and as suggested by Rob for another IP block: [0]) I am not sure if syscon parent really matches hardware implementation. In all our Networking SoCs, chiptop is kind of misc register collection. Some registers can't belong to any particular group, or they need to work together with other modules(therefore, these misc registers would be accessed by two or more modules). However, chiptop is not a hardware module. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible keeping regmap is great in my opinion because it's a nice API and gets rid of some boilerplate even better if it makes things easier for accessing the secure processor [...] 4. Code not optimized and intel internal review not assessed. insights from you (like the issue with the reset callback) are very valuable - this shows that we should focus on having one driver. Based on the above findings, I would suggest reset-lantiq.c to move to reset-intel-syscon.c my concern with having two separate drivers is that it will be hard to migrate from reset-lantiq to the "optimized" reset-intel-syscon driver. I don't have access to the datasheets for the any Lantiq/Intel SoC (VRX200 and even older). so debugging issues after switching from one driver to another is tedious because I cannot tell which part of the driver is causing a problem (it's either "all code from driver A" vs "all code from driver B", meaning it's hard to narrow it down). with separate commits/patches that are improving the reset-lantiq driver I can do git bisect to find the cause of a problem on the older SoCs (VRX200 for example) Our internal version supports XRX350/XRX500/PRX300(MIPS based) and latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c should be straight forward. what about the _reset callback on the XRX350/XRX5
RE: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Dilip, > -Original Message- > From: devicetree-ow...@vger.kernel.org ow...@vger.kernel.org> On Behalf Of Dilip Kota > Sent: Donnerstag, 19. September 2019 10:06 > To: Martin Blumenstingl > Cc: Chuan Hua, Lei ; Kim, Cheol Yong > ; devicet...@vger.kernel.org; linux- > ker...@vger.kernel.org; p.za...@pengutronix.de; Wu, Qiming ming...@intel.com>; r...@kernel.org; Hauke Mehrtens > Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM > SoC > > Hi Martin, > > On 9/12/2019 2:38 PM, Dilip Kota wrote: > > Re-sending the mail, because of delivery failure. > > sorry for the spam. > > > > Hi Martin, > > > > On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: > >> Hi, > >> > >> On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei > >> wrote: > >> [...] > >>>>>>>>>> I'm not surprised that we got some of the IP block layout for > >>>>>>>>>> the > >>>>>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old > >>>>>>>>>> Lantiq UGW > >>>>>>>>>> (BSP). > >>>>>>>>>> with proper documentation (as in a "public datasheet for the > >>>>>>>>>> SoC") it > >>>>>>>>>> would be easy to spot these mistakes (at least I assume that > the > >>>>>>>>>> quality of the Infineon / Lantiq datasheets is excellent). > >>>>>>>>>> > >>>>>>>>>> back to reset-intel-syscon: > >>>>>>>>>> assigning only one job to the RCU hardware is a good idea (in > >>>>>>>>>> my opinion). > >>>>>>>>>> that brings up a question: why do we need the "syscon" > >>>>>>>>>> compatible for > >>>>>>>>>> the RCU node? > >>>>>>>>>> this is typically used when registers are accessed by another > >>>>>>>>>> IP block > >>>>>>>>>> and the other driver has to access these registers as well. > >>>>>>>>>> does this > >>>>>>>>>> mean that there's more hidden in the RCU registers? > >>>>>>>>> As I mentioned, some other misc registers are put into RCU > >>>>>>>>> even they > >>>>>>>>> don't belong to reset functions. > >>>>>>>> OK, just be aware that there are also rules for syscon > compatible > >>>>>>>> drivers, see for example: [0] > >>>>>>>> if Rob (dt-bindings maintainer) is happy with the documentation > in > >>>>>>>> patch 1 then I'm fine with it as well. > >>>>>>>> for my own education I would appreciate if you could describe > >>>>>>>> these > >>>>>>>> "other misc registers" with a few sentences (I assume that this > >>>>>>>> can > >>>>>>>> also help Rob) > >>>>>>> For LGM, RCU is clean. There would be no MISC register after > >>>>>>> software's > >>>>>>> feedback. These misc registers will be moved to chiptop/misc > >>>>>>> groups(implemented by syscon). For legacy SoC, we do have a lot > >>>>>>> MISC > >>>>>>> registers for different SoCs. > >>>>>> OK, I think I understand now: chiptop != RCU > >>>>>> so RCU really only has one purpose: handling resets > >>>>>> while chiptop manages all the random bits > >>>>>> > >>>>>> does this means we don't need RCU to match "syscon"? > >>>>> If we don't support legacy SoC with the same driver, we don't need > >>>>> syscon, just regmap. Regmap is a must for us since we will use > regmap > >>>>> proxy to implement secure rest via secure processor. > >>>> I think we should drop the syscon compatible for LGM then > >>>> even for the legacy SoCs the reset controller should not have a > syscon > >>>> compatible: instead it should have a syscon parent (as the current > >>>> "lantiq,xrx200-reset" binding requires and as suggested by
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On 9/12/2019 2:38 PM, Dilip Kota wrote: Re-sending the mail, because of delivery failure. sorry for the spam. Hi Martin, On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: Hi, On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei wrote: [...] I'm not surprised that we got some of the IP block layout for the VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW (BSP). with proper documentation (as in a "public datasheet for the SoC") it would be easy to spot these mistakes (at least I assume that the quality of the Infineon / Lantiq datasheets is excellent). back to reset-intel-syscon: assigning only one job to the RCU hardware is a good idea (in my opinion). that brings up a question: why do we need the "syscon" compatible for the RCU node? this is typically used when registers are accessed by another IP block and the other driver has to access these registers as well. does this mean that there's more hidden in the RCU registers? As I mentioned, some other misc registers are put into RCU even they don't belong to reset functions. OK, just be aware that there are also rules for syscon compatible drivers, see for example: [0] if Rob (dt-bindings maintainer) is happy with the documentation in patch 1 then I'm fine with it as well. for my own education I would appreciate if you could describe these "other misc registers" with a few sentences (I assume that this can also help Rob) For LGM, RCU is clean. There would be no MISC register after software's feedback. These misc registers will be moved to chiptop/misc groups(implemented by syscon). For legacy SoC, we do have a lot MISC registers for different SoCs. OK, I think I understand now: chiptop != RCU so RCU really only has one purpose: handling resets while chiptop manages all the random bits does this means we don't need RCU to match "syscon"? If we don't support legacy SoC with the same driver, we don't need syscon, just regmap. Regmap is a must for us since we will use regmap proxy to implement secure rest via secure processor. I think we should drop the syscon compatible for LGM then even for the legacy SoCs the reset controller should not have a syscon compatible: instead it should have a syscon parent (as the current "lantiq,xrx200-reset" binding requires and as suggested by Rob for another IP block: [0]) I am not sure if syscon parent really matches hardware implementation. In all our Networking SoCs, chiptop is kind of misc register collection. Some registers can't belong to any particular group, or they need to work together with other modules(therefore, these misc registers would be accessed by two or more modules). However, chiptop is not a hardware module. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible keeping regmap is great in my opinion because it's a nice API and gets rid of some boilerplate even better if it makes things easier for accessing the secure processor [...] 4. Code not optimized and intel internal review not assessed. insights from you (like the issue with the reset callback) are very valuable - this shows that we should focus on having one driver. Based on the above findings, I would suggest reset-lantiq.c to move to reset-intel-syscon.c my concern with having two separate drivers is that it will be hard to migrate from reset-lantiq to the "optimized" reset-intel-syscon driver. I don't have access to the datasheets for the any Lantiq/Intel SoC (VRX200 and even older). so debugging issues after switching from one driver to another is tedious because I cannot tell which part of the driver is causing a problem (it's either "all code from driver A" vs "all code from driver B", meaning it's hard to narrow it down). with separate commits/patches that are improving the reset-lantiq driver I can do git bisect to find the cause of a problem on the older SoCs (VRX200 for example) Our internal version supports XRX350/XRX500/PRX300(MIPS based) and latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c should be straight forward. what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do they only use level resets (_assert and _deassert) or are some reset lines using reset pulses (_reset)? when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c we still had to add support for the _reset callback as this is missing in reset-intel-syscon.c currently Yes. We have reset pulse(assert, then check the reset status). only now I realized that the reset-intel-syscon driver does not seem to use the status registers (instead it's looking at the reset registers when checking the status). what happened to
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Re-sending the mail, because of delivery failure. sorry for the spam. Hi Martin, On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: Hi, On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei wrote: [...] I'm not surprised that we got some of the IP block layout for the VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW (BSP). with proper documentation (as in a "public datasheet for the SoC") it would be easy to spot these mistakes (at least I assume that the quality of the Infineon / Lantiq datasheets is excellent). back to reset-intel-syscon: assigning only one job to the RCU hardware is a good idea (in my opinion). that brings up a question: why do we need the "syscon" compatible for the RCU node? this is typically used when registers are accessed by another IP block and the other driver has to access these registers as well. does this mean that there's more hidden in the RCU registers? As I mentioned, some other misc registers are put into RCU even they don't belong to reset functions. OK, just be aware that there are also rules for syscon compatible drivers, see for example: [0] if Rob (dt-bindings maintainer) is happy with the documentation in patch 1 then I'm fine with it as well. for my own education I would appreciate if you could describe these "other misc registers" with a few sentences (I assume that this can also help Rob) For LGM, RCU is clean. There would be no MISC register after software's feedback. These misc registers will be moved to chiptop/misc groups(implemented by syscon). For legacy SoC, we do have a lot MISC registers for different SoCs. OK, I think I understand now: chiptop != RCU so RCU really only has one purpose: handling resets while chiptop manages all the random bits does this means we don't need RCU to match "syscon"? If we don't support legacy SoC with the same driver, we don't need syscon, just regmap. Regmap is a must for us since we will use regmap proxy to implement secure rest via secure processor. I think we should drop the syscon compatible for LGM then even for the legacy SoCs the reset controller should not have a syscon compatible: instead it should have a syscon parent (as the current "lantiq,xrx200-reset" binding requires and as suggested by Rob for another IP block: [0]) I am not sure if syscon parent really matches hardware implementation. In all our Networking SoCs, chiptop is kind of misc register collection. Some registers can't belong to any particular group, or they need to work together with other modules(therefore, these misc registers would be accessed by two or more modules). However, chiptop is not a hardware module. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible keeping regmap is great in my opinion because it's a nice API and gets rid of some boilerplate even better if it makes things easier for accessing the secure processor [...] 4. Code not optimized and intel internal review not assessed. insights from you (like the issue with the reset callback) are very valuable - this shows that we should focus on having one driver. Based on the above findings, I would suggest reset-lantiq.c to move to reset-intel-syscon.c my concern with having two separate drivers is that it will be hard to migrate from reset-lantiq to the "optimized" reset-intel-syscon driver. I don't have access to the datasheets for the any Lantiq/Intel SoC (VRX200 and even older). so debugging issues after switching from one driver to another is tedious because I cannot tell which part of the driver is causing a problem (it's either "all code from driver A" vs "all code from driver B", meaning it's hard to narrow it down). with separate commits/patches that are improving the reset-lantiq driver I can do git bisect to find the cause of a problem on the older SoCs (VRX200 for example) Our internal version supports XRX350/XRX500/PRX300(MIPS based) and latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c should be straight forward. what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do they only use level resets (_assert and _deassert) or are some reset lines using reset pulses (_reset)? when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c we still had to add support for the _reset callback as this is missing in reset-intel-syscon.c currently Yes. We have reset pulse(assert, then check the reset status). only now I realized that the reset-intel-syscon driver does not seem to use the status registers (instead it's looking at the reset registers when checking the status). what happened to the status registers - do they still exist in newer SoCs (like LGM)? why are they not used? Reset
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi, On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei wrote: [...] > >>> I'm not surprised that we got some of the IP block layout for the > >>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW > >>> (BSP). > >>> with proper documentation (as in a "public datasheet for the SoC") it > >>> would be easy to spot these mistakes (at least I assume that the > >>> quality of the Infineon / Lantiq datasheets is excellent). > >>> > >>> back to reset-intel-syscon: > >>> assigning only one job to the RCU hardware is a good idea (in my > >>> opinion). > >>> that brings up a question: why do we need the "syscon" compatible for > >>> the RCU node? > >>> this is typically used when registers are accessed by another IP block > >>> and the other driver has to access these registers as well. does this > >>> mean that there's more hidden in the RCU registers? > >> As I mentioned, some other misc registers are put into RCU even they > >> don't belong to reset functions. > > OK, just be aware that there are also rules for syscon compatible > > drivers, see for example: [0] > > if Rob (dt-bindings maintainer) is happy with the documentation in > > patch 1 then I'm fine with it as well. > > for my own education I would appreciate if you could describe these > > "other misc registers" with a few sentences (I assume that this can > > also help Rob) > For LGM, RCU is clean. There would be no MISC register after software's > feedback. These misc registers will be moved to chiptop/misc > groups(implemented by syscon). For legacy SoC, we do have a lot MISC > registers for different SoCs. > >>> OK, I think I understand now: chiptop != RCU > >>> so RCU really only has one purpose: handling resets > >>> while chiptop manages all the random bits > >>> > >>> does this means we don't need RCU to match "syscon"? > >> If we don't support legacy SoC with the same driver, we don't need > >> syscon, just regmap. Regmap is a must for us since we will use regmap > >> proxy to implement secure rest via secure processor. > > I think we should drop the syscon compatible for LGM then > > even for the legacy SoCs the reset controller should not have a syscon > > compatible: instead it should have a syscon parent (as the current > > "lantiq,xrx200-reset" binding requires and as suggested by Rob for > > another IP block: [0]) > I am not sure if syscon parent really matches hardware implementation. > In all our Networking SoCs, chiptop is kind of misc register collection. > Some registers can't belong to any particular group, or they need to > work together with other modules(therefore, these misc registers would > be accessed by two or more modules). However, chiptop is not a hardware > module. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible > > > > keeping regmap is great in my opinion because it's a nice API and gets > > rid of some boilerplate > > even better if it makes things easier for accessing the secure processor > > > > [...] > 4. Code not optimized and intel internal review not assessed. > >>> insights from you (like the issue with the reset callback) are > >>> very > >>> valuable - this shows that we should focus on having one driver. > >>> > Based on the above findings, I would suggest reset-lantiq.c to > move to > reset-intel-syscon.c > >>> my concern with having two separate drivers is that it will be > >>> hard to > >>> migrate from reset-lantiq to the "optimized" reset-intel-syscon > >>> driver. > >>> I don't have access to the datasheets for the any Lantiq/Intel SoC > >>> (VRX200 and even older). > >>> so debugging issues after switching from one driver to another is > >>> tedious because I cannot tell which part of the driver is causing > >>> a > >>> problem (it's either "all code from driver A" vs "all code from > >>> driver > >>> B", meaning it's hard to narrow it down). > >>> with separate commits/patches that are improving the reset-lantiq > >>> driver I can do git bisect to find the cause of a problem on the > >>> older > >>> SoCs (VRX200 for example) > >> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and > >> latest Lighting Mountain(X86 based). Migration to > >> reset-intel-syscon.c > >> should be straight forward. > > what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do > >
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On 9/3/2019 6:04 AM, Martin Blumenstingl wrote: Hi, On Mon, Sep 2, 2019 at 11:45 AM Chuan Hua, Lei wrote: Hi Martin, On 9/2/2019 5:38 AM, Martin Blumenstingl wrote: Hi, On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei wrote: Hi Martin, On 8/30/2019 5:40 AM, Martin Blumenstingl wrote: Hi, On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei wrote: I'm not surprised that we got some of the IP block layout for the VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW (BSP). with proper documentation (as in a "public datasheet for the SoC") it would be easy to spot these mistakes (at least I assume that the quality of the Infineon / Lantiq datasheets is excellent). back to reset-intel-syscon: assigning only one job to the RCU hardware is a good idea (in my opinion). that brings up a question: why do we need the "syscon" compatible for the RCU node? this is typically used when registers are accessed by another IP block and the other driver has to access these registers as well. does this mean that there's more hidden in the RCU registers? As I mentioned, some other misc registers are put into RCU even they don't belong to reset functions. OK, just be aware that there are also rules for syscon compatible drivers, see for example: [0] if Rob (dt-bindings maintainer) is happy with the documentation in patch 1 then I'm fine with it as well. for my own education I would appreciate if you could describe these "other misc registers" with a few sentences (I assume that this can also help Rob) For LGM, RCU is clean. There would be no MISC register after software's feedback. These misc registers will be moved to chiptop/misc groups(implemented by syscon). For legacy SoC, we do have a lot MISC registers for different SoCs. OK, I think I understand now: chiptop != RCU so RCU really only has one purpose: handling resets while chiptop manages all the random bits does this means we don't need RCU to match "syscon"? If we don't support legacy SoC with the same driver, we don't need syscon, just regmap. Regmap is a must for us since we will use regmap proxy to implement secure rest via secure processor. I think we should drop the syscon compatible for LGM then even for the legacy SoCs the reset controller should not have a syscon compatible: instead it should have a syscon parent (as the current "lantiq,xrx200-reset" binding requires and as suggested by Rob for another IP block: [0]) I am not sure if syscon parent really matches hardware implementation. In all our Networking SoCs, chiptop is kind of misc register collection. Some registers can't belong to any particular group, or they need to work together with other modules(therefore, these misc registers would be accessed by two or more modules). However, chiptop is not a hardware module. keeping regmap is great in my opinion because it's a nice API and gets rid of some boilerplate even better if it makes things easier for accessing the secure processor [...] 4. Code not optimized and intel internal review not assessed. insights from you (like the issue with the reset callback) are very valuable - this shows that we should focus on having one driver. Based on the above findings, I would suggest reset-lantiq.c to move to reset-intel-syscon.c my concern with having two separate drivers is that it will be hard to migrate from reset-lantiq to the "optimized" reset-intel-syscon driver. I don't have access to the datasheets for the any Lantiq/Intel SoC (VRX200 and even older). so debugging issues after switching from one driver to another is tedious because I cannot tell which part of the driver is causing a problem (it's either "all code from driver A" vs "all code from driver B", meaning it's hard to narrow it down). with separate commits/patches that are improving the reset-lantiq driver I can do git bisect to find the cause of a problem on the older SoCs (VRX200 for example) Our internal version supports XRX350/XRX500/PRX300(MIPS based) and latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c should be straight forward. what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do they only use level resets (_assert and _deassert) or are some reset lines using reset pulses (_reset)? when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c we still had to add support for the _reset callback as this is missing in reset-intel-syscon.c currently Yes. We have reset pulse(assert, then check the reset status). only now I realized that the reset-intel-syscon driver does not seem to use the status registers (instead it's looking at the reset registers when checking the status). what happened to the status registers - do they still exist in newer SoCs (like LGM)? why are they not used? Reset status check is there. regmap_read_poll_timeout to check status big. Status register offset <4) from request register. For legacy, there is one exception, we can add soc specific data to handle it.
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi, On Mon, Sep 2, 2019 at 11:45 AM Chuan Hua, Lei wrote: > > Hi Martin, > > > On 9/2/2019 5:38 AM, Martin Blumenstingl wrote: > > Hi, > > > > On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei > > wrote: > >> Hi Martin, > >> > >> On 8/30/2019 5:40 AM, Martin Blumenstingl wrote: > >>> Hi, > >>> > >>> On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei > >>> wrote: > >>> > > I'm not surprised that we got some of the IP block layout for the > > VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW > > (BSP). > > with proper documentation (as in a "public datasheet for the SoC") it > > would be easy to spot these mistakes (at least I assume that the > > quality of the Infineon / Lantiq datasheets is excellent). > > > > back to reset-intel-syscon: > > assigning only one job to the RCU hardware is a good idea (in my > > opinion). > > that brings up a question: why do we need the "syscon" compatible for > > the RCU node? > > this is typically used when registers are accessed by another IP block > > and the other driver has to access these registers as well. does this > > mean that there's more hidden in the RCU registers? > As I mentioned, some other misc registers are put into RCU even they > don't belong to reset functions. > >>> OK, just be aware that there are also rules for syscon compatible > >>> drivers, see for example: [0] > >>> if Rob (dt-bindings maintainer) is happy with the documentation in > >>> patch 1 then I'm fine with it as well. > >>> for my own education I would appreciate if you could describe these > >>> "other misc registers" with a few sentences (I assume that this can > >>> also help Rob) > >> For LGM, RCU is clean. There would be no MISC register after software's > >> feedback. These misc registers will be moved to chiptop/misc > >> groups(implemented by syscon). For legacy SoC, we do have a lot MISC > >> registers for different SoCs. > > OK, I think I understand now: chiptop != RCU > > so RCU really only has one purpose: handling resets > > while chiptop manages all the random bits > > > > does this means we don't need RCU to match "syscon"? > > If we don't support legacy SoC with the same driver, we don't need > syscon, just regmap. Regmap is a must for us since we will use regmap > proxy to implement secure rest via secure processor. I think we should drop the syscon compatible for LGM then even for the legacy SoCs the reset controller should not have a syscon compatible: instead it should have a syscon parent (as the current "lantiq,xrx200-reset" binding requires and as suggested by Rob for another IP block: [0]) keeping regmap is great in my opinion because it's a nice API and gets rid of some boilerplate even better if it makes things easier for accessing the secure processor > > > >>> [...] > >> 4. Code not optimized and intel internal review not assessed. > > insights from you (like the issue with the reset callback) are very > > valuable - this shows that we should focus on having one driver. > > > >> Based on the above findings, I would suggest reset-lantiq.c to > >> move to > >> reset-intel-syscon.c > > my concern with having two separate drivers is that it will be hard > > to > > migrate from reset-lantiq to the "optimized" reset-intel-syscon > > driver. > > I don't have access to the datasheets for the any Lantiq/Intel SoC > > (VRX200 and even older). > > so debugging issues after switching from one driver to another is > > tedious because I cannot tell which part of the driver is causing a > > problem (it's either "all code from driver A" vs "all code from > > driver > > B", meaning it's hard to narrow it down). > > with separate commits/patches that are improving the reset-lantiq > > driver I can do git bisect to find the cause of a problem on the > > older > > SoCs (VRX200 for example) > Our internal version supports XRX350/XRX500/PRX300(MIPS based) and > latest Lighting Mountain(X86 based). Migration to > reset-intel-syscon.c > should be straight forward. > >>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do > >>> they only use level resets (_assert and _deassert) or are some reset > >>> lines using reset pulses (_reset)? > >>> > >>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c > >>> we still had to add support for the _reset callback as this is missing > >>> in reset-intel-syscon.c currently > >> Yes. We have reset pulse(assert, then check the reset status). > > only now I realized that the reset-intel-syscon driver does not seem > > to use the status registers (instead it's looking at the reset > > registers when checking the status). > > what happened to the status registers -
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 9/2/2019 5:38 AM, Martin Blumenstingl wrote:
Hi,
On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
wrote:
Hi Martin,
On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
Hi,
On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
wrote:
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they
don't belong to reset functions.
OK, just be aware that there are also rules for syscon compatible
drivers, see for example: [0]
if Rob (dt-bindings maintainer) is happy with the documentation in
patch 1 then I'm fine with it as well.
for my own education I would appreciate if you could describe these
"other misc registers" with a few sentences (I assume that this can
also help Rob)
For LGM, RCU is clean. There would be no MISC register after software's
feedback. These misc registers will be moved to chiptop/misc
groups(implemented by syscon). For legacy SoC, we do have a lot MISC
registers for different SoCs.
OK, I think I understand now: chiptop != RCU
so RCU really only has one purpose: handling resets
while chiptop manages all the random bits
does this means we don't need RCU to match "syscon"?
If we don't support legacy SoC with the same driver, we don't need
syscon, just regmap. Regmap is a must for us since we will use regmap
proxy to implement secure rest via secure processor.
[...]
4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)
Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
should be straight forward.
what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
they only use level resets (_assert and _deassert) or are some reset
lines using reset pulses (_reset)?
when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
we still had to add support for the _reset callback as this is missing
in reset-intel-syscon.c currently
Yes. We have reset pulse(assert, then check the reset status).
only now I realized that the reset-intel-syscon driver does not seem
to use the status registers (instead it's looking at the reset
registers when checking the status).
what happened to the status registers - do they still exist in newer
SoCs (like LGM)? why are they not used?
Reset status check is there. regmap_read_poll_timeout to check status
big. Status register offset <4) from request register. For legacy, there
is one exception, we can add soc specific data to handle it.
I see, thank you for the explanation
this won't work on VRX200 for example because the status register is
not always at (reset register - 0x4)
As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
with one soc data in the compatible array.
For example(not same as upstream, but idea is similar)
static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
{
if (data->soc_data->legacy && req_off == RCU_RST_REQ)
return RCU_RST_STAT;
else
return req_off + 0x4;
}
on VRX200 for example there seem to be some cases where the bits in
the reset and status registers are different (for example: the first
GPHY seems to use reset bit 31 but status bit 30)
this is currently not supported in reset-intel-syscon
This is most tricky and ugly part for VRX200/Danube. Do you have any
idea to handle this nicely?
with reset-lantiq we have the following register information:
a) reset
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi,
On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
wrote:
>
> Hi Martin,
>
> On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
> > Hi,
> >
> > On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
> > wrote:
> >
> >>> I'm not surprised that we got some of the IP block layout for the
> >>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
> >>> (BSP).
> >>> with proper documentation (as in a "public datasheet for the SoC") it
> >>> would be easy to spot these mistakes (at least I assume that the
> >>> quality of the Infineon / Lantiq datasheets is excellent).
> >>>
> >>> back to reset-intel-syscon:
> >>> assigning only one job to the RCU hardware is a good idea (in my opinion).
> >>> that brings up a question: why do we need the "syscon" compatible for
> >>> the RCU node?
> >>> this is typically used when registers are accessed by another IP block
> >>> and the other driver has to access these registers as well. does this
> >>> mean that there's more hidden in the RCU registers?
> >> As I mentioned, some other misc registers are put into RCU even they
> >> don't belong to reset functions.
> > OK, just be aware that there are also rules for syscon compatible
> > drivers, see for example: [0]
> > if Rob (dt-bindings maintainer) is happy with the documentation in
> > patch 1 then I'm fine with it as well.
> > for my own education I would appreciate if you could describe these
> > "other misc registers" with a few sentences (I assume that this can
> > also help Rob)
> For LGM, RCU is clean. There would be no MISC register after software's
> feedback. These misc registers will be moved to chiptop/misc
> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
> registers for different SoCs.
OK, I think I understand now: chiptop != RCU
so RCU really only has one purpose: handling resets
while chiptop manages all the random bits
does this means we don't need RCU to match "syscon"?
> >
> > [...]
> 4. Code not optimized and intel internal review not assessed.
> >>> insights from you (like the issue with the reset callback) are very
> >>> valuable - this shows that we should focus on having one driver.
> >>>
> Based on the above findings, I would suggest reset-lantiq.c to move
> to
> reset-intel-syscon.c
> >>> my concern with having two separate drivers is that it will be hard to
> >>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> >>> driver.
> >>> I don't have access to the datasheets for the any Lantiq/Intel SoC
> >>> (VRX200 and even older).
> >>> so debugging issues after switching from one driver to another is
> >>> tedious because I cannot tell which part of the driver is causing a
> >>> problem (it's either "all code from driver A" vs "all code from driver
> >>> B", meaning it's hard to narrow it down).
> >>> with separate commits/patches that are improving the reset-lantiq
> >>> driver I can do git bisect to find the cause of a problem on the older
> >>> SoCs (VRX200 for example)
> >> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> >> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> >> should be straight forward.
> > what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> > they only use level resets (_assert and _deassert) or are some reset
> > lines using reset pulses (_reset)?
> >
> > when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> > we still had to add support for the _reset callback as this is missing
> > in reset-intel-syscon.c currently
> Yes. We have reset pulse(assert, then check the reset status).
> >>> only now I realized that the reset-intel-syscon driver does not seem
> >>> to use the status registers (instead it's looking at the reset
> >>> registers when checking the status).
> >>> what happened to the status registers - do they still exist in newer
> >>> SoCs (like LGM)? why are they not used?
> >> Reset status check is there. regmap_read_poll_timeout to check status
> >> big. Status register offset <4) from request register. For legacy, there
> >> is one exception, we can add soc specific data to handle it.
> > I see, thank you for the explanation
> > this won't work on VRX200 for example because the status register is
> > not always at (reset register - 0x4)
>
> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
> with one soc data in the compatible array.
>
> For example(not same as upstream, but idea is similar)
>
> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
> {
> if (data->soc_data->legacy && req_off == RCU_RST_REQ)
> return RCU_RST_STAT;
> else
> return req_off + 0x4;
> }
>
> >
> >>> on VRX200 for example there seem to be some cases where the bits in
> >>> the reset and status registers are different (for example: the first
> >>> GPHY seems
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
Hi,
On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
wrote:
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they
don't belong to reset functions.
OK, just be aware that there are also rules for syscon compatible
drivers, see for example: [0]
if Rob (dt-bindings maintainer) is happy with the documentation in
patch 1 then I'm fine with it as well.
for my own education I would appreciate if you could describe these
"other misc registers" with a few sentences (I assume that this can
also help Rob)
For LGM, RCU is clean. There would be no MISC register after software's
feedback. These misc registers will be moved to chiptop/misc
groups(implemented by syscon). For legacy SoC, we do have a lot MISC
registers for different SoCs.
[...]
4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)
Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
should be straight forward.
what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
they only use level resets (_assert and _deassert) or are some reset
lines using reset pulses (_reset)?
when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
we still had to add support for the _reset callback as this is missing
in reset-intel-syscon.c currently
Yes. We have reset pulse(assert, then check the reset status).
only now I realized that the reset-intel-syscon driver does not seem
to use the status registers (instead it's looking at the reset
registers when checking the status).
what happened to the status registers - do they still exist in newer
SoCs (like LGM)? why are they not used?
Reset status check is there. regmap_read_poll_timeout to check status
big. Status register offset <4) from request register. For legacy, there
is one exception, we can add soc specific data to handle it.
I see, thank you for the explanation
this won't work on VRX200 for example because the status register is
not always at (reset register - 0x4)
As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
with one soc data in the compatible array.
For example(not same as upstream, but idea is similar)
static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
{
if (data->soc_data->legacy && req_off == RCU_RST_REQ)
return RCU_RST_STAT;
else
return req_off + 0x4;
}
on VRX200 for example there seem to be some cases where the bits in
the reset and status registers are different (for example: the first
GPHY seems to use reset bit 31 but status bit 30)
this is currently not supported in reset-intel-syscon
This is most tricky and ugly part for VRX200/Danube. Do you have any
idea to handle this nicely?
with reset-lantiq we have the following register information:
a) reset offset: first reg property
b) status offset: second reg property
c) reset bit: first #reset-cell
d) status bit: second #reset-cell
reset-intel-syscon derives half of this information from the two #reset-cells:
a) reset offset: first #reset-cell
b) status offset: reset offset - 0x4
c) reset bit: second #reset-cell
d) status bit: same as reset bit
I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
driver because I don't know enough about LGM (yet).
on VRX200 my understanding is that we have
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi, On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei wrote: > > > > I'm not surprised that we got some of the IP block layout for the > > VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW > > (BSP). > > with proper documentation (as in a "public datasheet for the SoC") it > > would be easy to spot these mistakes (at least I assume that the > > quality of the Infineon / Lantiq datasheets is excellent). > > > > back to reset-intel-syscon: > > assigning only one job to the RCU hardware is a good idea (in my opinion). > > that brings up a question: why do we need the "syscon" compatible for > > the RCU node? > > this is typically used when registers are accessed by another IP block > > and the other driver has to access these registers as well. does this > > mean that there's more hidden in the RCU registers? > As I mentioned, some other misc registers are put into RCU even they > don't belong to reset functions. OK, just be aware that there are also rules for syscon compatible drivers, see for example: [0] if Rob (dt-bindings maintainer) is happy with the documentation in patch 1 then I'm fine with it as well. for my own education I would appreciate if you could describe these "other misc registers" with a few sentences (I assume that this can also help Rob) [...] > >> 4. Code not optimized and intel internal review not assessed. > > insights from you (like the issue with the reset callback) are very > > valuable - this shows that we should focus on having one driver. > > > >> Based on the above findings, I would suggest reset-lantiq.c to move to > >> reset-intel-syscon.c > > my concern with having two separate drivers is that it will be hard to > > migrate from reset-lantiq to the "optimized" reset-intel-syscon > > driver. > > I don't have access to the datasheets for the any Lantiq/Intel SoC > > (VRX200 and even older). > > so debugging issues after switching from one driver to another is > > tedious because I cannot tell which part of the driver is causing a > > problem (it's either "all code from driver A" vs "all code from driver > > B", meaning it's hard to narrow it down). > > with separate commits/patches that are improving the reset-lantiq > > driver I can do git bisect to find the cause of a problem on the older > > SoCs (VRX200 for example) > Our internal version supports XRX350/XRX500/PRX300(MIPS based) and > latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c > should be straight forward. > >>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do > >>> they only use level resets (_assert and _deassert) or are some reset > >>> lines using reset pulses (_reset)? > >>> > >>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c > >>> we still had to add support for the _reset callback as this is missing > >>> in reset-intel-syscon.c currently > >> Yes. We have reset pulse(assert, then check the reset status). > > only now I realized that the reset-intel-syscon driver does not seem > > to use the status registers (instead it's looking at the reset > > registers when checking the status). > > what happened to the status registers - do they still exist in newer > > SoCs (like LGM)? why are they not used? > Reset status check is there. regmap_read_poll_timeout to check status > big. Status register offset <4) from request register. For legacy, there > is one exception, we can add soc specific data to handle it. I see, thank you for the explanation this won't work on VRX200 for example because the status register is not always at (reset register - 0x4) > > on VRX200 for example there seem to be some cases where the bits in > > the reset and status registers are different (for example: the first > > GPHY seems to use reset bit 31 but status bit 30) > > this is currently not supported in reset-intel-syscon > This is most tricky and ugly part for VRX200/Danube. Do you have any > idea to handle this nicely? with reset-lantiq we have the following register information: a) reset offset: first reg property b) status offset: second reg property c) reset bit: first #reset-cell d) status bit: second #reset-cell reset-intel-syscon derives half of this information from the two #reset-cells: a) reset offset: first #reset-cell b) status offset: reset offset - 0x4 c) reset bit: second #reset-cell d) status bit: same as reset bit I cannot make any suggestion (yet) how to handle VRX200 and LGM in one driver because I don't know enough about LGM (yet). on VRX200 my understanding is that we have 64 reset bits (2x 32bit registers) and 64 status bits (also 2x 32bit registers). each reset bit has a corresponding status bit but the numbering may be different it's not clear to me how many resets LGM supports and how they are organized. for example: I think it makes a difference if "there are 64 registers with each one reset bit" versus "there are two registers with
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
On 8/29/2019 4:01 AM, Martin Blumenstingl wrote:
Hi,
On Wed, Aug 28, 2019 at 3:53 AM Chuan Hua, Lei
wrote:
[...]
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
in reset-lantiq.c, we split each reset request /status pair into one
reset controller.
Each reset controller handles up to 32 resets. It will create up to 9
even more
reset controllers in the new SoCs. In reality, there is only one RCU
controller for all
SoCs. These designs worked but did not follow what hardware implemented.
After checking the existing code and referring to other implementation,
we decided to
use register offset + bit position method. It can support all SoCs with
this methods
without code change(device tree change only).
maybe I have a different interpretation of what "RCU" does.
let me explain it in my own words based on my knowledge about VRX200:
- in my own words it is a multi function device with the following
functionality:
- it contains two reset controllers (reset at 0x10, status 0x14 and
reset at 0x48, status at 0x24)
- it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
and PHY registers at 0x34, ANA cfg at 0x3c)
- it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
- it contains endianness configuration registers (for PCI, PCIe, ...)
- it contains the watchdog boot status (whether the SoC was previously
reset by the WDT)
- maybe more, but I don't know anything else about it
In fact, there is only one reset controller for all SoCs even it doesn't
prevent software from virtualizing multiple reset controllers. Reset
control does include some misc stuff which has been moved to chiptop in
new SoCs so that RCU has a clean job.
just to confirm that I understand this correctly:
even the VRX200 SoC only has one physical reset controller?
instead of a contiguous register area (let's say: 0x10 to 0x1c) it
uses four separate registers:
- 0x10 for asserting/deasserting/pulsing the first 32 reset lines
- 0x14 for the status of the first 32 reset lines
- 0x48 for asserting/deasserting/pulsing the second 32 reset lines
- 0x28 for the status of the second 32 reset lines
Yes, but for VRX200, reset controller registers include some other misc
registers. At that time,
hardware doesn't use chiptop concept, they put some misc registers into
CGU/RCU which makes it quite messy.
We also prefer to have 0x10~0x1c. However, when developing VRX200, 0x18,
0x20 and other address had been used by other registers. system becomes
more complex, need more reset bits for new modules, then hardware just
added them to any available place. From another angle, hardware people
also tried to keep backward compatible with old products like Danube.
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they
don't belong to reset functions. In MIPS, global software reset handled
in arch/mips/, only recently, this situation changed. This means we have
at least two places to access this module.
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
reboot {
compatible = "syscon-reboot";
regmap = <>;
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi,
On Wed, Aug 28, 2019 at 3:53 AM Chuan Hua, Lei
wrote:
[...]
> 1. reset-lantiq.c use index instead of register offset + bit position.
> index reset is good for a small system (< 64). However, it will become
> very
> difficult to use if you have > 100 reset. So we use register offset +
> bit position
> >>> reset-lantiq uses bit bit positions for specifying the reset line.
> >>> for example this is from OpenWrt's vr9.dtsi:
> >>> reset0: reset-controller@10 {
> >>> ...
> >>> reg = <0x10 4>, <0x14 4>;
> >>> #reset-cells = <2>;
> >>> };
> >>>
> >>> gphy0: gphy@20 {
> >>> ...
> >>> resets = < 31 30>, < 7 7>;
> >>> reset-names = "gphy", "gphy2";
> >>> };
> >>>
> >>> in my own words this means:
> >>> - all reset0 reset bits are at offset 0x10 (parent is RCU)
> >>> - all reset0 status bits are at offset 0x14 (parent is RCU)
> >>> - the first reset line uses reset bit 31 and status bit 30
> >>> - the second reset line uses reset bit 7 and status bit 7
> >>> - there can be multiple reset-controller instances, each taking the
> >>> reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
> >>> reset controller "reset1" with reset offset 0x48 and status offset
> >>> 0x24)
> >> in reset-lantiq.c, we split each reset request /status pair into one
> >> reset controller.
> >>
> >> Each reset controller handles up to 32 resets. It will create up to 9
> >> even more
> >> reset controllers in the new SoCs. In reality, there is only one RCU
> >> controller for all
> >> SoCs. These designs worked but did not follow what hardware implemented.
> >>
> >> After checking the existing code and referring to other implementation,
> >> we decided to
> >> use register offset + bit position method. It can support all SoCs with
> >> this methods
> >> without code change(device tree change only).
> > maybe I have a different interpretation of what "RCU" does.
> > let me explain it in my own words based on my knowledge about VRX200:
> > - in my own words it is a multi function device with the following
> > functionality:
> > - it contains two reset controllers (reset at 0x10, status 0x14 and
> > reset at 0x48, status at 0x24)
> > - it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
> > and PHY registers at 0x34, ANA cfg at 0x3c)
> > - it contains the configuration for the two GPHY IP blocks (at 0x20 and
> > 0x68)
> > - it contains endianness configuration registers (for PCI, PCIe, ...)
> > - it contains the watchdog boot status (whether the SoC was previously
> > reset by the WDT)
> > - maybe more, but I don't know anything else about it
> In fact, there is only one reset controller for all SoCs even it doesn't
> prevent software from virtualizing multiple reset controllers. Reset
> control does include some misc stuff which has been moved to chiptop in
> new SoCs so that RCU has a clean job.
just to confirm that I understand this correctly:
even the VRX200 SoC only has one physical reset controller?
instead of a contiguous register area (let's say: 0x10 to 0x1c) it
uses four separate registers:
- 0x10 for asserting/deasserting/pulsing the first 32 reset lines
- 0x14 for the status of the first 32 reset lines
- 0x48 for asserting/deasserting/pulsing the second 32 reset lines
- 0x28 for the status of the second 32 reset lines
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
> 2. reset-lantiq.c does not support device restart which is part of the
> reset in
> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
> >>> it was moved to the .dts instead of the arch code. again from
> >>> OpenWrt's vr9.dtsi [0]:
> >>> reboot {
> >>> compatible = "syscon-reboot";
> >>> regmap = <>;
> >>> offset = <0x10>;
> >>> mask = <0xe000>;
> >>> };
> >>>
> >>> this sets the reset0 reset bits 31, 30 and 29 at reboot
> >> ok. but not sure why we need to reset bit 31 and 29. global softwre
> >> reset is bit 30.
> > I don't know either. depending on what the LGM SoCs need you can
> > change the "mask" property to the value that fits that SoC best
> >
> > [...]
> All SoCs have only one global software reset bit.
OK
you can still use syscon-reboot to set the soft reset bit if Rob
(dt-binding maintainer) doesn't like the
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 8/28/2019 5:15 AM, Martin Blumenstingl wrote:
Hi,
On Tue, Aug 27, 2019 at 4:23 AM Chuan Hua, Lei
wrote:
[...]
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
in reset-lantiq.c, we split each reset request /status pair into one
reset controller.
Each reset controller handles up to 32 resets. It will create up to 9
even more
reset controllers in the new SoCs. In reality, there is only one RCU
controller for all
SoCs. These designs worked but did not follow what hardware implemented.
After checking the existing code and referring to other implementation,
we decided to
use register offset + bit position method. It can support all SoCs with
this methods
without code change(device tree change only).
maybe I have a different interpretation of what "RCU" does.
let me explain it in my own words based on my knowledge about VRX200:
- in my own words it is a multi function device with the following
functionality:
- it contains two reset controllers (reset at 0x10, status 0x14 and
reset at 0x48, status at 0x24)
- it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
and PHY registers at 0x34, ANA cfg at 0x3c)
- it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
- it contains endianness configuration registers (for PCI, PCIe, ...)
- it contains the watchdog boot status (whether the SoC was previously
reset by the WDT)
- maybe more, but I don't know anything else about it
In fact, there is only one reset controller for all SoCs even it doesn't
prevent software from virtualizing multiple reset controllers. Reset
control does include some misc stuff which has been moved to chiptop in
new SoCs so that RCU has a clean job.
we tried our best to document this in
Documentation/devicetree/bindings/mips/lantiq/rcu.txt
I'm not sure about the details of the RCU on the LGM SoCs:
if it contains more than just reset controllers then please let Rob
Herring (dt-bindings maintainer) know about this.
we may only have one chance to do it right, if we start with a
"broken" binding then devices with incompatible bootloaders etc. may
have already shipped
(in general: that is why the devicetree maintainers want to have all
device properties documented in the binding, even if the driver does
not support all of them yet)
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
reboot {
compatible = "syscon-reboot";
regmap = <>;
offset = <0x10>;
mask = <0xe000>;
};
this sets the reset0 reset bits 31, 30 and 29 at reboot
ok. but not sure why we need to reset bit 31 and 29. global softwre
reset is bit 30.
I don't know either. depending on what the LGM SoCs need you can
change the "mask" property to the value that fits that SoC best
[...]
All SoCs have only one global software reset bit.
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)
Yes, this is called hardware reset. We can't control reset duration.
is this the same for all, old and new SoCs?
New SoCs have removed support for hardware reset after software's feedback.
Old SoCs such as VRX200/ARX300 has both software/hardware resets
nice, it's good to see teamwork between hardware and software teams!
[...]
4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi,
On Tue, Aug 27, 2019 at 4:23 AM Chuan Hua, Lei
wrote:
[...]
> >> 1. reset-lantiq.c use index instead of register offset + bit position.
> >> index reset is good for a small system (< 64). However, it will become very
> >> difficult to use if you have > 100 reset. So we use register offset +
> >> bit position
> > reset-lantiq uses bit bit positions for specifying the reset line.
> > for example this is from OpenWrt's vr9.dtsi:
> >reset0: reset-controller@10 {
> > ...
> > reg = <0x10 4>, <0x14 4>;
> > #reset-cells = <2>;
> >};
> >
> >gphy0: gphy@20 {
> > ...
> > resets = < 31 30>, < 7 7>;
> > reset-names = "gphy", "gphy2";
> >};
> >
> > in my own words this means:
> > - all reset0 reset bits are at offset 0x10 (parent is RCU)
> > - all reset0 status bits are at offset 0x14 (parent is RCU)
> > - the first reset line uses reset bit 31 and status bit 30
> > - the second reset line uses reset bit 7 and status bit 7
> > - there can be multiple reset-controller instances, each taking the
> > reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
> > reset controller "reset1" with reset offset 0x48 and status offset
> > 0x24)
>
> in reset-lantiq.c, we split each reset request /status pair into one
> reset controller.
>
> Each reset controller handles up to 32 resets. It will create up to 9
> even more
> reset controllers in the new SoCs. In reality, there is only one RCU
> controller for all
> SoCs. These designs worked but did not follow what hardware implemented.
>
> After checking the existing code and referring to other implementation,
> we decided to
> use register offset + bit position method. It can support all SoCs with
> this methods
> without code change(device tree change only).
maybe I have a different interpretation of what "RCU" does.
let me explain it in my own words based on my knowledge about VRX200:
- in my own words it is a multi function device with the following
functionality:
- it contains two reset controllers (reset at 0x10, status 0x14 and
reset at 0x48, status at 0x24)
- it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
and PHY registers at 0x34, ANA cfg at 0x3c)
- it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
- it contains endianness configuration registers (for PCI, PCIe, ...)
- it contains the watchdog boot status (whether the SoC was previously
reset by the WDT)
- maybe more, but I don't know anything else about it
we tried our best to document this in
Documentation/devicetree/bindings/mips/lantiq/rcu.txt
I'm not sure about the details of the RCU on the LGM SoCs:
if it contains more than just reset controllers then please let Rob
Herring (dt-bindings maintainer) know about this.
we may only have one chance to do it right, if we start with a
"broken" binding then devices with incompatible bootloaders etc. may
have already shipped
(in general: that is why the devicetree maintainers want to have all
device properties documented in the binding, even if the driver does
not support all of them yet)
> >> 2. reset-lantiq.c does not support device restart which is part of the
> >> reset in
> >> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
> > it was moved to the .dts instead of the arch code. again from
> > OpenWrt's vr9.dtsi [0]:
> >reboot {
> > compatible = "syscon-reboot";
> > regmap = <>;
> > offset = <0x10>;
> > mask = <0xe000>;
> >};
> >
> > this sets the reset0 reset bits 31, 30 and 29 at reboot
> ok. but not sure why we need to reset bit 31 and 29. global softwre
> reset is bit 30.
I don't know either. depending on what the LGM SoCs need you can
change the "mask" property to the value that fits that SoC best
[...]
> > - other reset lines only support reset pulses. the _reset function
> > should be used in this case
> > - the _reset function should only assert the reset line, then wait
> > until the hardware automatically de-asserts it (without any further
> > write)
> Yes, this is called hardware reset. We can't control reset duration.
> > is this the same for all, old and new SoCs?
>
> New SoCs have removed support for hardware reset after software's feedback.
>
> Old SoCs such as VRX200/ARX300 has both software/hardware resets
nice, it's good to see teamwork between hardware and software teams!
[...]
> >> 4. Code not optimized and intel internal review not assessed.
> > insights from you (like the issue with the reset callback) are very
> > valuable - this shows that we should focus on having one driver.
> >
> >> Based on the above findings, I would suggest reset-lantiq.c to move to
> >> reset-intel-syscon.c
> > my concern with having two separate drivers is that it will be hard to
> > migrate from reset-lantiq to the "optimized" reset-intel-syscon
> > driver.
> > I don't have access to the datasheets for the any Lantiq/Intel SoC
> > (VRX200 and even older).
> > so debugging issues after switching from one driver
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
Please check the reply below.
On 8/27/2019 5:49 AM, Martin Blumenstingl wrote:
Hi,
On Mon, Aug 26, 2019 at 6:01 AM Chuan Hua, Lei
wrote:
Hi Martin,
Thanks for your comment.
thank you for the quick reply
On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
Hi Dilip,
Add driver for the reset controller present on Intel
Lightening Mountain (LGM) SoC for performing reset
management of the devices present on the SoC. Driver also
registers a reset handler to peform the entire device reset.
[...]
+static const struct of_device_id intel_reset_match[] = {
+{ .compatible = "intel,rcu-lgm" },
+{}
+};
how is this IP block differnet from the one used in many Lantiq SoCs?
there is already an upstream driver for the RCU IP block on the Lantiq
SoCs: drivers/reset/reset-lantiq.c
some background:
Lantiq was started as a spinoff from Infineon in 2009. Intel then
acquired Lantiq in 2015. source: [0]
Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
(Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
typically used for PON/GPON/ADSL/VDSL capable network devices).
Thus I think it is likely that the new "Lightening Mountain" SoCs use
an updated version of the Lantiq RCU IP.
I would not say there is a fundamental difference since reset is a
really simple
stuff from all reset drivers. However, it did have some difference
from existing reset-lantiq.c since SoC becomes more and more complex.
OK, let me go through your detailed list
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
in reset-lantiq.c, we split each reset request /status pair into one
reset controller.
Each reset controller handles up to 32 resets. It will create up to 9
even more
reset controllers in the new SoCs. In reality, there is only one RCU
controller for all
SoCs. These designs worked but did not follow what hardware implemented.
After checking the existing code and referring to other implementation,
we decided to
use register offset + bit position method. It can support all SoCs with
this methods
without code change(device tree change only).
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
reboot {
compatible = "syscon-reboot";
regmap = <>;
offset = <0x10>;
mask = <0xe000>;
};
this sets the reset0 reset bits 31, 30 and 29 at reboot
ok. but not sure why we need to reset bit 31 and 29. global softwre
reset is bit 30.
3. reset-lantiqc reset callback doesn't implement what hardware implemented
function. In old SoCs, some bits in the same register can be hardware
reset clear.
It just call assert + assert. For these SoCs, we should only call
assert, hardware will auto deassert.
I didn't know that. so to confirm I understand this correctly:
- some reset lines must be asserted and de-asserted manually (setting
and clearing the bit manually). the _assert and _deassert functions
should be used in this case
Yes. We call software managed reset. callers call assert, deassert and delay
duration according to their specific requirement.
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)
Yes, this is called hardware reset. We can't control reset duration.
is this the same for all, old and new SoCs?
New SoCs have removed support for hardware reset after software's feedback.
Old SoCs such as VRX200/ARX300 has both software/hardware resets
only two mainline Lantiq drivers are using reset lines - both are
using the _assert and _deassert variants:
- drivers/net/dsa/lantiq_gswip.c
- drivers/phy/lantiq/phy-lantiq-rcu-usb2.c
It means migration will be very easy:)
4. Code not optimized and intel internal review not
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi,
On Mon, Aug 26, 2019 at 6:01 AM Chuan Hua, Lei
wrote:
>
> Hi Martin,
>
> Thanks for your comment.
thank you for the quick reply
> On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
> > Hi Dilip,
> >
> >> Add driver for the reset controller present on Intel
> >> Lightening Mountain (LGM) SoC for performing reset
> >> management of the devices present on the SoC. Driver also
> >> registers a reset handler to peform the entire device reset.
> > [...]
> >> +static const struct of_device_id intel_reset_match[] = {
> >> +{ .compatible = "intel,rcu-lgm" },
> >> +{}
> >> +};
> > how is this IP block differnet from the one used in many Lantiq SoCs?
> > there is already an upstream driver for the RCU IP block on the Lantiq
> > SoCs: drivers/reset/reset-lantiq.c
> >
> > some background:
> > Lantiq was started as a spinoff from Infineon in 2009. Intel then
> > acquired Lantiq in 2015. source: [0]
> > Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
> > (Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
> > typically used for PON/GPON/ADSL/VDSL capable network devices).
> > Thus I think it is likely that the new "Lightening Mountain" SoCs use
> > an updated version of the Lantiq RCU IP.
>
> I would not say there is a fundamental difference since reset is a
> really simple
> stuff from all reset drivers. However, it did have some difference
> from existing reset-lantiq.c since SoC becomes more and more complex.
OK, let me go through your detailed list
> 1. reset-lantiq.c use index instead of register offset + bit position.
> index reset is good for a small system (< 64). However, it will become very
> difficult to use if you have > 100 reset. So we use register offset +
> bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
> 2. reset-lantiq.c does not support device restart which is part of the
> reset in
> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
reboot {
compatible = "syscon-reboot";
regmap = <>;
offset = <0x10>;
mask = <0xe000>;
};
this sets the reset0 reset bits 31, 30 and 29 at reboot
> 3. reset-lantiqc reset callback doesn't implement what hardware implemented
> function. In old SoCs, some bits in the same register can be hardware
> reset clear.
>
> It just call assert + assert. For these SoCs, we should only call
> assert, hardware will auto deassert.
I didn't know that. so to confirm I understand this correctly:
- some reset lines must be asserted and de-asserted manually (setting
and clearing the bit manually). the _assert and _deassert functions
should be used in this case
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)
is this the same for all, old and new SoCs?
only two mainline Lantiq drivers are using reset lines - both are
using the _assert and _deassert variants:
- drivers/net/dsa/lantiq_gswip.c
- drivers/phy/lantiq/phy-lantiq-rcu-usb2.c
> 4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
> Based on the above findings, I would suggest reset-lantiq.c to move to
> reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)
> What is your opinion?
I explained why I would like to avoid having two separate drivers
(even just for a limited amount of time)
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
On 8/23/2019 6:09 PM, Philipp Zabel wrote: On Fri, 2019-08-23 at 17:47 +0800, Dilip Kota wrote: [...] Thanks for pointing it out. Reset is not supported on LGM platform. I will update the reset_device() definition to "return -EOPNOTSUPP" In that case you can just drop intel_reset_device() completely, the core checks whether ops->reset is set before trying to call it. Agree, will do the same. Regards, Dilip regards Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
Thanks for your comment.
On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
Hi Dilip,
Add driver for the reset controller present on Intel
Lightening Mountain (LGM) SoC for performing reset
management of the devices present on the SoC. Driver also
registers a reset handler to peform the entire device reset.
[...]
+static const struct of_device_id intel_reset_match[] = {
+ { .compatible = "intel,rcu-lgm" },
+ {}
+};
how is this IP block differnet from the one used in many Lantiq SoCs?
there is already an upstream driver for the RCU IP block on the Lantiq
SoCs: drivers/reset/reset-lantiq.c
some background:
Lantiq was started as a spinoff from Infineon in 2009. Intel then
acquired Lantiq in 2015. source: [0]
Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
(Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
typically used for PON/GPON/ADSL/VDSL capable network devices).
Thus I think it is likely that the new "Lightening Mountain" SoCs use
an updated version of the Lantiq RCU IP.
I would not say there is a fundamental difference since reset is a
really simple
stuff from all reset drivers. However, it did have some difference
from existing reset-lantiq.c since SoC becomes more and more complex.
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
3. reset-lantiqc reset callback doesn't implement what hardware implemented
function. In old SoCs, some bits in the same register can be hardware
reset clear.
It just call assert + assert. For these SoCs, we should only call
assert, hardware
will auto deassert.
4. Code not optimized and intel internal review not assessed.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
What is your opinion?
Chuanhua
Martin
[0] https://wikidevi.com/wiki/Lantiq
RE: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Dilip,
> Add driver for the reset controller present on Intel
> Lightening Mountain (LGM) SoC for performing reset
> management of the devices present on the SoC. Driver also
> registers a reset handler to peform the entire device reset.
[...]
> +static const struct of_device_id intel_reset_match[] = {
> + { .compatible = "intel,rcu-lgm" },
> + {}
> +};
how is this IP block differnet from the one used in many Lantiq SoCs?
there is already an upstream driver for the RCU IP block on the Lantiq
SoCs: drivers/reset/reset-lantiq.c
some background:
Lantiq was started as a spinoff from Infineon in 2009. Intel then
acquired Lantiq in 2015. source: [0]
Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
(Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
typically used for PON/GPON/ADSL/VDSL capable network devices).
Thus I think it is likely that the new "Lightening Mountain" SoCs use
an updated version of the Lantiq RCU IP.
Martin
[0] https://wikidevi.com/wiki/Lantiq
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
On Fri, 2019-08-23 at 17:47 +0800, Dilip Kota wrote: [...] > Thanks for pointing it out. > Reset is not supported on LGM platform. > I will update the reset_device() definition to "return -EOPNOTSUPP" In that case you can just drop intel_reset_device() completely, the core checks whether ops->reset is set before trying to call it. regards Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Philipp,
On 8/23/2019 4:43 PM, Philipp Zabel wrote:
Hi Dilip,
On Fri, 2019-08-23 at 13:28 +0800, Dilip Kota wrote:
Add driver for the reset controller present on Intel
Lightening Mountain (LGM) SoC for performing reset
management of the devices present on the SoC. Driver also
registers a reset handler to peform the entire device reset.
Signed-off-by: Dilip Kota
thank you for the patch, I have a few questions/suggestions below:
---
Changes on v2:
No changes
drivers/reset/Kconfig | 10 ++
drivers/reset/Makefile | 1 +
drivers/reset/reset-intel-syscon.c | 215 +
3 files changed, 226 insertions(+)
create mode 100644 drivers/reset/reset-intel-syscon.c
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 6d5d76db55b0..e0fd14cb4cf5 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -64,6 +64,16 @@ config RESET_IMX7
help
This enables the reset controller driver for i.MX7 SoCs.
+config RESET_INTEL_SYSCON
+ bool "Intel SYSCON Reset Driver"
+ depends on HAS_IOMEM
+ select MFD_SYSCON
+ help
+ This enables the reset driver support for Intel SoC devices with
+ memory-mapped reset registers as part of a syscon device node. If
+ you wish to use the reset framework for such memory-mapped devices,
+ say Y here. Otherwise, say N.
Is this driver really as generic as this description makes it sound,
or is it limited to LGM?
Do you expect this to be reused by other platforms? The timeouts,
status register offsets, and readback mechanism might be platform
specific.
Yes you are correct, this is platform specific limited to LGM.
I will update the config description.
+
config RESET_LANTIQ
bool "Lantiq XWAY Reset Driver" if COMPILE_TEST
default SOC_TYPE_XWAY
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 61456b8f659c..6d68c50c7e89 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
+obj-$(CONFIG_RESET_INTEL_SYSCON) += reset-intel-syscon.o
obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
obj-$(CONFIG_RESET_MESON) += reset-meson.o
diff --git a/drivers/reset/reset-intel-syscon.c
b/drivers/reset/reset-intel-syscon.c
new file mode 100644
index ..6377a0cac1e7
--- /dev/null
+++ b/drivers/reset/reset-intel-syscon.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Intel Corporation.
+ * Lei Chuanhua
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct intel_reset_data {
+ struct reset_controller_dev rcdev;
+ struct notifier_block restart_nb;
+ struct device *dev;
+ struct regmap *regmap;
+ u32 reboot_id;
+};
+
+/* reset platform data */
+#define to_reset_data(x) container_of(x, struct intel_reset_data, rcdev)
+
+/*
+ * Reset status register offset relative to
+ * the reset control register(X) is X + 4
+ */
+static inline u32 id_to_reg_bit_and_offset(unsigned long id,
+ u32 *regbit, u32 *regoff)
+{
+ *regoff = id >> 8;
+ *regbit = id & 0x1f;
+ return *regoff + 0x4;
+}
+
+static int intel_set_clr_bits(struct intel_reset_data *data,
+ unsigned long id, bool set, u64 timeout)
+{
+ u32 regoff, regbit;
+ u32 stat_off;
+ u32 val;
+ int ret;
+
+ stat_off = id_to_reg_bit_and_offset(id, , );
+
+ val = set ? BIT(regbit) : 0;
+ ret = regmap_update_bits(data->regmap, regoff, BIT(regbit), val);
+ if (ret)
+ return ret;
+
+ return regmap_read_poll_timeout(data->regmap, stat_off, val,
+ set == !!(val & BIT(regbit)),
+ 20, timeout);
+}
+
+static int intel_assert_device(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ struct intel_reset_data *data = to_reset_data(rcdev);
+ int ret;
+
+ ret = intel_set_clr_bits(data, id, 1, 200);
Since set is of type bool, I'd use true instead of 1.
Agree, will update it to true/false at all the places.
Good Catch!
+ if (ret)
+ dev_err(data->dev, "Failed to set reset assert bit %d\n", ret);
+ return ret;
+}
+
[...]
+
+static int intel_reset_device(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ struct intel_reset_data *data = to_reset_data(rcdev);
+ int ret;
+
+ ret = intel_set_clr_bits(data, id, 1, 2);
+ if (ret)
+ dev_err(data->dev, "Failed to reset device %d\n", ret);
+ return ret;
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Dilip,
On Fri, 2019-08-23 at 13:28 +0800, Dilip Kota wrote:
> Add driver for the reset controller present on Intel
> Lightening Mountain (LGM) SoC for performing reset
> management of the devices present on the SoC. Driver also
> registers a reset handler to peform the entire device reset.
>
> Signed-off-by: Dilip Kota
thank you for the patch, I have a few questions/suggestions below:
> ---
> Changes on v2:
> No changes
>
> drivers/reset/Kconfig | 10 ++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-intel-syscon.c | 215
> +
> 3 files changed, 226 insertions(+)
> create mode 100644 drivers/reset/reset-intel-syscon.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 6d5d76db55b0..e0fd14cb4cf5 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -64,6 +64,16 @@ config RESET_IMX7
> help
> This enables the reset controller driver for i.MX7 SoCs.
>
> +config RESET_INTEL_SYSCON
> + bool "Intel SYSCON Reset Driver"
> + depends on HAS_IOMEM
> + select MFD_SYSCON
> + help
> + This enables the reset driver support for Intel SoC devices with
> + memory-mapped reset registers as part of a syscon device node. If
> + you wish to use the reset framework for such memory-mapped devices,
> + say Y here. Otherwise, say N.
Is this driver really as generic as this description makes it sound,
or is it limited to LGM?
Do you expect this to be reused by other platforms? The timeouts,
status register offsets, and readback mechanism might be platform
specific.
> +
> config RESET_LANTIQ
> bool "Lantiq XWAY Reset Driver" if COMPILE_TEST
> default SOC_TYPE_XWAY
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 61456b8f659c..6d68c50c7e89 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> +obj-$(CONFIG_RESET_INTEL_SYSCON) += reset-intel-syscon.o
> obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> obj-$(CONFIG_RESET_MESON) += reset-meson.o
> diff --git a/drivers/reset/reset-intel-syscon.c
> b/drivers/reset/reset-intel-syscon.c
> new file mode 100644
> index ..6377a0cac1e7
> --- /dev/null
> +++ b/drivers/reset/reset-intel-syscon.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Intel Corporation.
> + * Lei Chuanhua
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct intel_reset_data {
> + struct reset_controller_dev rcdev;
> + struct notifier_block restart_nb;
> + struct device *dev;
> + struct regmap *regmap;
> + u32 reboot_id;
> +};
> +
> +/* reset platform data */
> +#define to_reset_data(x) container_of(x, struct intel_reset_data, rcdev)
> +
> +/*
> + * Reset status register offset relative to
> + * the reset control register(X) is X + 4
> + */
> +static inline u32 id_to_reg_bit_and_offset(unsigned long id,
> +u32 *regbit, u32 *regoff)
> +{
> + *regoff = id >> 8;
> + *regbit = id & 0x1f;
> + return *regoff + 0x4;
> +}
> +
> +static int intel_set_clr_bits(struct intel_reset_data *data,
> + unsigned long id, bool set, u64 timeout)
> +{
> + u32 regoff, regbit;
> + u32 stat_off;
> + u32 val;
> + int ret;
> +
> + stat_off = id_to_reg_bit_and_offset(id, , );
> +
> + val = set ? BIT(regbit) : 0;
> + ret = regmap_update_bits(data->regmap, regoff, BIT(regbit), val);
> + if (ret)
> + return ret;
> +
> + return regmap_read_poll_timeout(data->regmap, stat_off, val,
> + set == !!(val & BIT(regbit)),
> + 20, timeout);
> +}
> +
> +static int intel_assert_device(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct intel_reset_data *data = to_reset_data(rcdev);
> + int ret;
> +
> + ret = intel_set_clr_bits(data, id, 1, 200);
Since set is of type bool, I'd use true instead of 1.
> + if (ret)
> + dev_err(data->dev, "Failed to set reset assert bit %d\n", ret);
> + return ret;
> +}
> +
[...]
> +
> +static int intel_reset_device(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct intel_reset_data *data = to_reset_data(rcdev);
> + int ret;
> +
> + ret = intel_set_clr_bits(data, id, 1, 2);
> + if (ret)
> + dev_err(data->dev, "Failed to reset device %d\n", ret);
> + return ret;
> +}
This doesn't seem right. _assert and _reset are doing exactly
