Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Alexey.
On 2 March 2018 at 14:51, Alexey Brodkin wrote:
>
> Hi Simon,
>
> On Thu, 2018-02-22 at 10:29 -0700, Simon Glass wrote:
> > Hi Alexey,
> >
> > On 22 February 2018 at 09:23, Alexey Brodkin
> > wrote:
> > > Hi Simon,
> > >
> > > On Thu, 2018-02-22 at 09:17 -0700, Simon Glass wrote:
>
> [snip]
>
> > > > I think a separate driver might be better, unless we want to make the
> > > > read/write interface go through regmap or similar?
> > >
> > > But in case of ARC's AUX regs portmap won't help because those AUX regs
> > > are
> > > couldn't be mapped - that a completely different address space and we may
> > > only access them via dedicated instructions (LR vs LD and SR vs ST).
> >
> > Well...
> >
> > 1. With a separate driver, you can do whatever you want :-) I know it
> > introduces code duplication though...
>
> Exactly I hate to introduce another driver which will be 99,9% the same
> as an existing one and then we'll need to care of it as well.
>
> > 2. With regmap you can add your own regmap driver, and again do
> > whatever you want. I can help with that if it sounds attractive
>
> Ok so I took a look at regmap in Linux kernel and indeed that
> will solve our problem: we'll have a regmap-mmio.c and regmap-arcaux.c
> with appropriate implementation of accessors but I'm not really sure
> it worth the trouble. Or your idea was to move all the different #ifdefs from
> serial_{in|out}_shift() to the corresponding regmap implementations such that
> we have something like below:
> regmap-mem32-le.c
> regmap-mem32-be.c
> regmap-portmapped.c
> etc
> ?
Yes, that's it.
I'm not sure why that driver is so special. Presumably other drivers
would have the same problem?
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Simon,
On Thu, 2018-02-22 at 10:29 -0700, Simon Glass wrote:
> Hi Alexey,
>
> On 22 February 2018 at 09:23, Alexey Brodkin
> wrote:
> > Hi Simon,
> >
> > On Thu, 2018-02-22 at 09:17 -0700, Simon Glass wrote:
[snip]
> > > I think a separate driver might be better, unless we want to make the
> > > read/write interface go through regmap or similar?
> >
> > But in case of ARC's AUX regs portmap won't help because those AUX regs are
> > couldn't be mapped - that a completely different address space and we may
> > only access them via dedicated instructions (LR vs LD and SR vs ST).
>
> Well...
>
> 1. With a separate driver, you can do whatever you want :-) I know it
> introduces code duplication though...
Exactly I hate to introduce another driver which will be 99,9% the same
as an existing one and then we'll need to care of it as well.
> 2. With regmap you can add your own regmap driver, and again do
> whatever you want. I can help with that if it sounds attractive
Ok so I took a look at regmap in Linux kernel and indeed that
will solve our problem: we'll have a regmap-mmio.c and regmap-arcaux.c
with appropriate implementation of accessors but I'm not really sure
it worth the trouble. Or your idea was to move all the different #ifdefs from
serial_{in|out}_shift() to the corresponding regmap implementations such that
we have something like below:
regmap-mem32-le.c
regmap-mem32-be.c
regmap-portmapped.c
etc
?
Regards,
Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Alexey, On 23 February 2018 at 11:08, Alexey Brodkin wrote: > Hi Simon, > > On Thu, 2018-02-22 at 10:29 -0700, Simon Glass wrote: >> Hi Alexey, >> >> On 22 February 2018 at 09:23, Alexey Brodkin >> wrote: >> > Hi Simon, >> > >> > On Thu, 2018-02-22 at 09:17 -0700, Simon Glass wrote: >> > > Hi, >> > > >> > > On 21 February 2018 at 05:26, Alexey Brodkin >> > > wrote: > > [snip] > >> > > > --- >> > > > drivers/serial/ns16550.c | 11 +-- >> > > > 1 file changed, 9 insertions(+), 2 deletions(-) >> > > >> > > I think a separate driver might be better, unless we want to make the >> > > read/write interface go through regmap or similar? >> > >> > But in case of ARC's AUX regs portmap won't help because those AUX regs are >> > couldn't be mapped - that a completely different address space and we may >> > only access them via dedicated instructions (LR vs LD and SR vs ST). >> >> Well... >> >> 1. With a separate driver, you can do whatever you want :-) I know it >> introduces code duplication though... >> >> 2. With regmap you can add your own regmap driver, and again do >> whatever you want. I can help with that if it sounds attractive > > That sounds definitely interesting! > Are there any pointers on how that usually done? Not yet. My ideas: - Add a new regmap_init_xxx() function - Adjust the regmap structure to allow read()/write() function pointers to be provided - Adjust the regmap code to call them, instead of the hard-coded memory-mapped ones - Adjust ns16550 to call regmap instead of readl/writel This should clean up the mess in ns16550 a bit. But it will require a bit of investigation / thinking. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Simon, On Thu, 2018-02-22 at 10:29 -0700, Simon Glass wrote: > Hi Alexey, > > On 22 February 2018 at 09:23, Alexey Brodkin > wrote: > > Hi Simon, > > > > On Thu, 2018-02-22 at 09:17 -0700, Simon Glass wrote: > > > Hi, > > > > > > On 21 February 2018 at 05:26, Alexey Brodkin > > > wrote: [snip] > > > > --- > > > > drivers/serial/ns16550.c | 11 +-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > I think a separate driver might be better, unless we want to make the > > > read/write interface go through regmap or similar? > > > > But in case of ARC's AUX regs portmap won't help because those AUX regs are > > couldn't be mapped - that a completely different address space and we may > > only access them via dedicated instructions (LR vs LD and SR vs ST). > > Well... > > 1. With a separate driver, you can do whatever you want :-) I know it > introduces code duplication though... > > 2. With regmap you can add your own regmap driver, and again do > whatever you want. I can help with that if it sounds attractive That sounds definitely interesting! Are there any pointers on how that usually done? -Alexey ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Alexey,
On 22 February 2018 at 09:23, Alexey Brodkin
wrote:
> Hi Simon,
>
> On Thu, 2018-02-22 at 09:17 -0700, Simon Glass wrote:
>> Hi,
>>
>> On 21 February 2018 at 05:26, Alexey Brodkin
>> wrote:
>> > Synopsys Data Fusion subsystem (DFSS) is targeted to deeply built-in
>> > use-cases and so to save some silicon area decision was made to
>> > escape usage of any busses and use instead directly wired to CPU
>> > peripherals. And one of those is DW APB UART.
>> >
>> > Later DFSS became a part of larger and more complicated SoCs with
>> > some other peripherals connected via common buses but default UART
>> > is still used via ARC core's auxulary registers which are not mapped
>> > to "normal" address space and we use very special instructions to access
>> > them, thus we cannot simply reuse whatever accessor we have in 16550
>> > UART as of now.
>> >
>> > Also we cannot just switch inb()/outb() to access ARC AUX regs always
>> > for DFSS because other peripherals have normal memory-mapped control
>> > registers and we need to use normal accessors for them.
>> >
>> > Frankly I don't like a lot what I did here but otherwise if I create
>> > a special driver for this I'll need to reimplement
>> > ns16550_serial_ops.putc()/getc() which will be pure copy-paseted from
>> > ns16550.c because the only difference is only in
>> > ns16550_{read|write}b().
>> >
>> > As mentioned above we cannot remap those auxiliary registers to
>> > normal memory address-space and thus we have to use very special
>> > accessors write_aux_reg()/read_aux_reg() that directly use special
>> > CPU intructions (namely LR/SR) for dealing with ARC AUX regs.
>> >
>> > Also note here I just use a check for a particular SoC being selected
>> > (CONFIG_ARCH_DFSS will be introduced shortly) but that is done just for
>> > simplicity, otherwise it might be a slecial Kconfig option for NS16550
>> > or anything else.
>> >
>> > I'd like to know what people think about possible colutions here.
>> > And as always any comments are much appreciated!
>> >
>> > Signed-off-by: Alexey Brodkin
>> > Cc: Simon Glass
>> > Cc: Tom Rini
>> > Cc: Stefan Roese
>> > ---
>> > drivers/serial/ns16550.c | 11 +--
>> > 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> I think a separate driver might be better, unless we want to make the
>> read/write interface go through regmap or similar?
>
> But in case of ARC's AUX regs portmap won't help because those AUX regs are
> couldn't be mapped - that a completely different address space and we may
> only access them via dedicated instructions (LR vs LD and SR vs ST).
Well...
1. With a separate driver, you can do whatever you want :-) I know it
introduces code duplication though...
2. With regmap you can add your own regmap driver, and again do
whatever you want. I can help with that if it sounds attractive
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Simon,
On Thu, 2018-02-22 at 09:17 -0700, Simon Glass wrote:
> Hi,
>
> On 21 February 2018 at 05:26, Alexey Brodkin
> wrote:
> > Synopsys Data Fusion subsystem (DFSS) is targeted to deeply built-in
> > use-cases and so to save some silicon area decision was made to
> > escape usage of any busses and use instead directly wired to CPU
> > peripherals. And one of those is DW APB UART.
> >
> > Later DFSS became a part of larger and more complicated SoCs with
> > some other peripherals connected via common buses but default UART
> > is still used via ARC core's auxulary registers which are not mapped
> > to "normal" address space and we use very special instructions to access
> > them, thus we cannot simply reuse whatever accessor we have in 16550
> > UART as of now.
> >
> > Also we cannot just switch inb()/outb() to access ARC AUX regs always
> > for DFSS because other peripherals have normal memory-mapped control
> > registers and we need to use normal accessors for them.
> >
> > Frankly I don't like a lot what I did here but otherwise if I create
> > a special driver for this I'll need to reimplement
> > ns16550_serial_ops.putc()/getc() which will be pure copy-paseted from
> > ns16550.c because the only difference is only in
> > ns16550_{read|write}b().
> >
> > As mentioned above we cannot remap those auxiliary registers to
> > normal memory address-space and thus we have to use very special
> > accessors write_aux_reg()/read_aux_reg() that directly use special
> > CPU intructions (namely LR/SR) for dealing with ARC AUX regs.
> >
> > Also note here I just use a check for a particular SoC being selected
> > (CONFIG_ARCH_DFSS will be introduced shortly) but that is done just for
> > simplicity, otherwise it might be a slecial Kconfig option for NS16550
> > or anything else.
> >
> > I'd like to know what people think about possible colutions here.
> > And as always any comments are much appreciated!
> >
> > Signed-off-by: Alexey Brodkin
> > Cc: Simon Glass
> > Cc: Tom Rini
> > Cc: Stefan Roese
> > ---
> > drivers/serial/ns16550.c | 11 +--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
>
> I think a separate driver might be better, unless we want to make the
> read/write interface go through regmap or similar?
But in case of ARC's AUX regs portmap won't help because those AUX regs are
couldn't be mapped - that a completely different address space and we may
only access them via dedicated instructions (LR vs LD and SR vs ST).
-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
On Thu, Feb 22, 2018 at 04:07:39PM +, Alexey Brodkin wrote:
> Hi Tom,
>
> On Thu, 2018-02-22 at 10:43 -0500, Tom Rini wrote:
> > On Wed, Feb 21, 2018 at 03:26:05PM +0300, Alexey Brodkin wrote:
>
> [snip]
>
> > > static inline void serial_out_shift(void *addr, int shift, int value)
> > > {
> > > -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> > > +#ifdef CONFIG_ARCH_DFSS
> > > + write_aux_reg((int)addr, value);
> > > +#elif defined(CONFIG_SYS_NS16550_PORT_MAPPED)
> > > outb(value, (ulong)addr);
> > > #elif defined(CONFIG_SYS_NS16550_MEM32) &&
> > > !defined(CONFIG_SYS_BIG_ENDIAN)
> > > out_le32(addr, value);
> > > @@ -70,7 +75,9 @@ static inline void serial_out_shift(void *addr, int
> > > shift, int value)
> > >
> > > static inline int serial_in_shift(void *addr, int shift)
> > > {
> > > -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> > > +#ifdef CONFIG_ARCH_DFSS
> > > + return read_aux_reg((int)addr);
> > > +#elif defined(CONFIG_SYS_NS16550_PORT_MAPPED)
> > > return inb((ulong)addr);
> > > #elif defined(CONFIG_SYS_NS16550_MEM32) &&
> > > !defined(CONFIG_SYS_BIG_ENDIAN)
> > > return in_le32(addr);
> >
> > As always, thanks for the detailed explanation. Yes, I think that of
> > the options, putting the details in read/write_aux_reg (and please make
> > sure read/write_aux_reg have a good function comment too) is the best
> > choice. Thanks!
>
> Frankly I didn't understand your comment well enough :)
> Do you suggest to keep proposed implementation (i.e. modification of
> serial_{in|out}_shift()) but add comments on what do we do and why or
> you really meant something completely different?
I'm fine with what you have above, in the driver. I missed that
read_aux_reg already exists, sorry. So I guess a detailed commit
message will have to be enough documentation for future reviewers of the
code. Or, we'll see what comes out of Simon's comments.
--
Tom
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi,
On 21 February 2018 at 05:26, Alexey Brodkin
wrote:
> Synopsys Data Fusion subsystem (DFSS) is targeted to deeply built-in
> use-cases and so to save some silicon area decision was made to
> escape usage of any busses and use instead directly wired to CPU
> peripherals. And one of those is DW APB UART.
>
> Later DFSS became a part of larger and more complicated SoCs with
> some other peripherals connected via common buses but default UART
> is still used via ARC core's auxulary registers which are not mapped
> to "normal" address space and we use very special instructions to access
> them, thus we cannot simply reuse whatever accessor we have in 16550
> UART as of now.
>
> Also we cannot just switch inb()/outb() to access ARC AUX regs always
> for DFSS because other peripherals have normal memory-mapped control
> registers and we need to use normal accessors for them.
>
> Frankly I don't like a lot what I did here but otherwise if I create
> a special driver for this I'll need to reimplement
> ns16550_serial_ops.putc()/getc() which will be pure copy-paseted from
> ns16550.c because the only difference is only in
> ns16550_{read|write}b().
>
> As mentioned above we cannot remap those auxiliary registers to
> normal memory address-space and thus we have to use very special
> accessors write_aux_reg()/read_aux_reg() that directly use special
> CPU intructions (namely LR/SR) for dealing with ARC AUX regs.
>
> Also note here I just use a check for a particular SoC being selected
> (CONFIG_ARCH_DFSS will be introduced shortly) but that is done just for
> simplicity, otherwise it might be a slecial Kconfig option for NS16550
> or anything else.
>
> I'd like to know what people think about possible colutions here.
> And as always any comments are much appreciated!
>
> Signed-off-by: Alexey Brodkin
> Cc: Simon Glass
> Cc: Tom Rini
> Cc: Stefan Roese
> ---
> drivers/serial/ns16550.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
I think a separate driver might be better, unless we want to make the
read/write interface go through regmap or similar?
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
Hi Tom,
On Thu, 2018-02-22 at 10:43 -0500, Tom Rini wrote:
> On Wed, Feb 21, 2018 at 03:26:05PM +0300, Alexey Brodkin wrote:
[snip]
> > static inline void serial_out_shift(void *addr, int shift, int value)
> > {
> > -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> > +#ifdef CONFIG_ARCH_DFSS
> > + write_aux_reg((int)addr, value);
> > +#elif defined(CONFIG_SYS_NS16550_PORT_MAPPED)
> > outb(value, (ulong)addr);
> > #elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
> > out_le32(addr, value);
> > @@ -70,7 +75,9 @@ static inline void serial_out_shift(void *addr, int
> > shift, int value)
> >
> > static inline int serial_in_shift(void *addr, int shift)
> > {
> > -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> > +#ifdef CONFIG_ARCH_DFSS
> > + return read_aux_reg((int)addr);
> > +#elif defined(CONFIG_SYS_NS16550_PORT_MAPPED)
> > return inb((ulong)addr);
> > #elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
> > return in_le32(addr);
>
> As always, thanks for the detailed explanation. Yes, I think that of
> the options, putting the details in read/write_aux_reg (and please make
> sure read/write_aux_reg have a good function comment too) is the best
> choice. Thanks!
Frankly I didn't understand your comment well enough :)
Do you suggest to keep proposed implementation (i.e. modification of
serial_{in|out}_shift()) but add comments on what do we do and why or
you really meant something completely different?
-Alexey
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC] ns16550: Add support for AUX regs usage on some ARC SoCs
On Wed, Feb 21, 2018 at 03:26:05PM +0300, Alexey Brodkin wrote:
> Synopsys Data Fusion subsystem (DFSS) is targeted to deeply built-in
> use-cases and so to save some silicon area decision was made to
> escape usage of any busses and use instead directly wired to CPU
> peripherals. And one of those is DW APB UART.
>
> Later DFSS became a part of larger and more complicated SoCs with
> some other peripherals connected via common buses but default UART
> is still used via ARC core's auxulary registers which are not mapped
> to "normal" address space and we use very special instructions to access
> them, thus we cannot simply reuse whatever accessor we have in 16550
> UART as of now.
>
> Also we cannot just switch inb()/outb() to access ARC AUX regs always
> for DFSS because other peripherals have normal memory-mapped control
> registers and we need to use normal accessors for them.
>
> Frankly I don't like a lot what I did here but otherwise if I create
> a special driver for this I'll need to reimplement
> ns16550_serial_ops.putc()/getc() which will be pure copy-paseted from
> ns16550.c because the only difference is only in
> ns16550_{read|write}b().
>
> As mentioned above we cannot remap those auxiliary registers to
> normal memory address-space and thus we have to use very special
> accessors write_aux_reg()/read_aux_reg() that directly use special
> CPU intructions (namely LR/SR) for dealing with ARC AUX regs.
>
> Also note here I just use a check for a particular SoC being selected
> (CONFIG_ARCH_DFSS will be introduced shortly) but that is done just for
> simplicity, otherwise it might be a slecial Kconfig option for NS16550
> or anything else.
>
> I'd like to know what people think about possible colutions here.
> And as always any comments are much appreciated!
>
> Signed-off-by: Alexey Brodkin
> Cc: Simon Glass
> Cc: Tom Rini
> Cc: Stefan Roese
> ---
> drivers/serial/ns16550.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 6f9ce689cfff..75f342f337f1 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -13,6 +13,9 @@
> #include
> #include
> #include
> +#ifdef CONFIG_ARCH_DFSS
> +#include
> +#endif
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -53,7 +56,9 @@ DECLARE_GLOBAL_DATA_PTR;
>
> static inline void serial_out_shift(void *addr, int shift, int value)
> {
> -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +#ifdef CONFIG_ARCH_DFSS
> + write_aux_reg((int)addr, value);
> +#elif defined(CONFIG_SYS_NS16550_PORT_MAPPED)
> outb(value, (ulong)addr);
> #elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
> out_le32(addr, value);
> @@ -70,7 +75,9 @@ static inline void serial_out_shift(void *addr, int shift,
> int value)
>
> static inline int serial_in_shift(void *addr, int shift)
> {
> -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +#ifdef CONFIG_ARCH_DFSS
> + return read_aux_reg((int)addr);
> +#elif defined(CONFIG_SYS_NS16550_PORT_MAPPED)
> return inb((ulong)addr);
> #elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_SYS_BIG_ENDIAN)
> return in_le32(addr);
As always, thanks for the detailed explanation. Yes, I think that of
the options, putting the details in read/write_aux_reg (and please make
sure read/write_aux_reg have a good function comment too) is the best
choice. Thanks!
--
Tom
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
