[U-Boot] microblaze: usable uart16550 for big endian systems

Hi Stephan,

>> Stephan Linz wrote:
>>> As a result of the commit 6833260 the uart16550 driver
>>> is broken for Microblaze big endian systems, because of
>>> the missing 3 byte offset. Other than as described, the
>>> U-Boot BSP does not treat properly the 3 byte offset.
>>>
>>> However, with the new 32 bit access to ns16550 registers
>>> we can enable correct register access for Microblaze big
>>> and little endian systems in the same manner.
>> The reason why I have applied that patch is that baseaddress generation
>> was moved to u-boot BSP out of u-boot configs.
>>
>> Here is example how addresses are generated.
>> BE system:
>> #define XILINX_UART16550
>> #define XILINX_UART16550_BASEADDR	0x83e00003
> 
> Hi Michal,
> 
> Who is generating this entry (especially incl. this offset)? Was it the
> MDL environment from PetaLinux? If so, which version?

u-boot BSP.

> I use my own MDL environment from TPOS, and the generator for
> xparameters.h does not add this offset, see proc put_uart16550_cfg():
> 
> https://gitorious.org/mbref/mbref/blobs/master/edk-repository/ThirdParty/lib/tpos_misclib.tcl#line1384
> 
> And seriously we never need this offset. With a sane endianess handling
> in software we will access the right bytes in uart16550. The Xilinx FPGA
> synthesis produce results that are good enough for us. All NS16550 8 bit
> registers alligned on 32 bit memory access: 0x000000rr on BE and
> 0xrr000000 in LE.
> 
> 
> The BSP generator (Xilinx MDL part) may never knows specifics about
> software or unclean code. Moreover we have to change the code ;-)
> 
> 
> Till now we have set CONFIG_SYS_NS16550_REG_SIZE to -4 and a offset of 3
> to the NS16550 base address for Microblaze BE systems. As I can see in
> ns16550.h that was completely wrong, or not? See:
> 
> #if !defined(CONFIG_SYS_NS16550_REG_SIZE)
> #error "Please define NS16550 registers size."
> #elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
> #define UART_REG(x)                                               \
>         unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1];\
>         unsigned char x;
> #elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
> #define UART_REG(x)     \
>         unsigned char x;\
>         unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
> #endif
> 
> struct NS16550 {
>         UART_REG(rbr);          /* 0 */
>         UART_REG(ier);          /* 1 */
> 
> ... and so on. For BE systems we should use CONFIG_SYS_NS16550_REG_SIZE
> set to 4 --> have a 3 byte gap on NS16550 base address and then point to
> the right byte on offset 3, or not? On LE systems we need to set -4 for
> *_REG_SIZE --> have a 3 byte gap after and betweeen each 8 bit
> registers.
> 
>> Anyway you solution looks interesting and I will test it.
> 
> However since commit 79df120 we can use direct 32 bit access to 8 bit
> NS16550 registers without gap generation in ns16550.h ... we need sane
> in_*/out_* implementation.
> 

I have look at it and tested on BE/LE. For 32bit accesses we need to implement
in/out_le32 functions which we don't have right now that's why please remove this macro
from your patch.

Our BSP generates/ed +3 offset that's why I prefer to mask it in the same patch
to be sure that baseaddr is correct and compatible with old versions.

Here is patch I have used. Please add that changes to v2 patch.

Thanks,
Michal

Am Mittwoch, den 23.11.2011, 15:40 +0100 schrieb Michal Simek: 
> Hi Stephan,
> 
> >> Stephan Linz wrote:
> >>> As a result of the commit 6833260 the uart16550 driver
> >>> is broken for Microblaze big endian systems, because of
> >>> the missing 3 byte offset. Other than as described, the
> >>> U-Boot BSP does not treat properly the 3 byte offset.
> >>>
> >>> However, with the new 32 bit access to ns16550 registers
> >>> we can enable correct register access for Microblaze big
> >>> and little endian systems in the same manner.
> >> The reason why I have applied that patch is that baseaddress generation
> >> was moved to u-boot BSP out of u-boot configs.
> >>
> >> --snip--
> > 
> >> Anyway you solution looks interesting and I will test it.
> > 
> > However since commit 79df120 we can use direct 32 bit access to 8 bit
> > NS16550 registers without gap generation in ns16550.h ... we need sane
> > in_*/out_* implementation.
> > 
> 
> I have look at it and tested on BE/LE. For 32bit accesses we need to implement
> in/out_le32 functions which we don't have right now

Hi Michal,

Oh yes, of course. There are no *_le32 operations, not yet.

> that's why please remove this macro
> from your patch.
> 
> Our BSP generates/ed +3 offset that's why I prefer to mask it in the same patch
> to be sure that baseaddr is correct and compatible with old versions.
> 
> Here is patch I have used. Please add that changes to v2 patch.

I'll do it this way. Give me a little time to change and test it.
Currently I am sill working on refactoring of the LL TEMAC driver. I
hope, the refactored driver than can merge in mainline ...


Thanks,
Stephan

> diff --git a/include/configs/microblaze-generic.h b/include/configs/microblaze-generic.h
> index b740a28..8085130 100644
> --- a/include/configs/microblaze-generic.h
> +++ b/include/configs/microblaze-generic.h
> @@ -55,10 +55,16 @@
>   #elif XILINX_UART16550_BASEADDR
>   # define CONFIG_SYS_NS16550            1
>   # define CONFIG_SYS_NS16550_SERIAL
> +
> +#if defined(__MICROBLAZEEL__)
>   # define CONFIG_SYS_NS16550_REG_SIZE   -4
> +#else
> +# define CONFIG_SYS_NS16550_REG_SIZE   4
> +#endif
> +
>   # define CONFIG_CONS_INDEX             1
>   # define CONFIG_SYS_NS16550_COM1 \
> -                       (XILINX_UART16550_BASEADDR + 0x1000)
> +                       ((XILINX_UART16550_BASEADDR & ~0xF) + 0x1000)
>   # define CONFIG_SYS_NS16550_CLK        XILINX_UART16550_CLOCK_HZ
>   # define CONFIG_BAUDRATE       115200
> 
> 
>

Stephan Linz wrote:
> Am Mittwoch, den 23.11.2011, 15:40 +0100 schrieb Michal Simek: 
>> Hi Stephan,
>>
>>>> Stephan Linz wrote:
>>>>> As a result of the commit 6833260 the uart16550 driver
>>>>> is broken for Microblaze big endian systems, because of
>>>>> the missing 3 byte offset. Other than as described, the
>>>>> U-Boot BSP does not treat properly the 3 byte offset.
>>>>>
>>>>> However, with the new 32 bit access to ns16550 registers
>>>>> we can enable correct register access for Microblaze big
>>>>> and little endian systems in the same manner.
>>>> The reason why I have applied that patch is that baseaddress generation
>>>> was moved to u-boot BSP out of u-boot configs.
>>>>
>>>> --snip--
>>>> Anyway you solution looks interesting and I will test it.
>>> However since commit 79df120 we can use direct 32 bit access to 8 bit
>>> NS16550 registers without gap generation in ns16550.h ... we need sane
>>> in_*/out_* implementation.
>>>
>> I have look at it and tested on BE/LE. For 32bit accesses we need to implement
>> in/out_le32 functions which we don't have right now
> 
> Hi Michal,
> 
> Oh yes, of course. There are no *_le32 operations, not yet.
> 
>> that's why please remove this macro
>> from your patch.
>>
>> Our BSP generates/ed +3 offset that's why I prefer to mask it in the same patch
>> to be sure that baseaddr is correct and compatible with old versions.
>>
>> Here is patch I have used. Please add that changes to v2 patch.
> 
> I'll do it this way. Give me a little time to change and test it.
> Currently I am sill working on refactoring of the LL TEMAC driver. I
> hope, the refactored driver than can merge in mainline ...

I have done it some time ago and look at "[PATCH v3] net: ll_temac: Add LL TEMAC driver to u-boot thread".
I have attached the latest version I have and I am not going to change it to follow
"new" u-boot network driver style because  I would like to keep ppc dcr support.

Thanks,
Michal

Am Donnerstag, den 24.11.2011, 20:13 +0100 schrieb Michal Simek:
> Stephan Linz wrote:
> >--snip--
> >>
> >> Here is patch I have used. Please add that changes to v2 patch.
> > 
> > I'll do it this way. Give me a little time to change and test it.
> > Currently I am sill working on refactoring of the LL TEMAC driver. I
> > hope, the refactored driver than can merge in mainline ...
> 
> I have done it some time ago and look at "[PATCH v3] net: ll_temac:
> Add LL TEMAC driver to u-boot thread".

Hi Michal,

yes, I've read the whole thread.

> I have attached the latest version I have and I am not going to change
> it to follow "new" u-boot network driver style

Hm, but it is possible ...

> because  I would like to keep ppc dcr support.

Even this was the main work I've done last week. I've split the driver
code into differnt sub modules (xilinx_ll_temac_fifo and
xilinx_ll_temac_sdram), introduce some call back functions into the sub
modules to harmonize the main driver code and divide the sdma code into
the two different bus access methodes: direct 32 bit memory access
(Microblaze) and indirect access via DCR (Xilinx PPC4xx).

Please, give me some time to finish the refactoring. I'll release all
results as fast as possible her in U-Boot list.

But, one issue was uncleare to me. Some functions, for example
xps_ll_temac_hostif_set(), were prepared to set bit 10 of CTL register.
But CTL bit 10 is not defined by any LL TEMAC documentation. Why did you
done this?


Thanks,
Stephan

> 
> Thanks,
> Michal
> 
> -- 
> Michal Simek, Ing. (M.Eng)
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel 2.6 Microblaze Linux -
> http://www.monstr.eu/fdt/
> Microblaze U-BOOT custodian

Stephan Linz wrote:
> Am Donnerstag, den 24.11.2011, 20:13 +0100 schrieb Michal Simek:
>> Stephan Linz wrote:
>>> --snip--
>>>> Here is patch I have used. Please add that changes to v2 patch.
>>> I'll do it this way. Give me a little time to change and test it.
>>> Currently I am sill working on refactoring of the LL TEMAC driver. I
>>> hope, the refactored driver than can merge in mainline ...
>> I have done it some time ago and look at "[PATCH v3] net: ll_temac:
>> Add LL TEMAC driver to u-boot thread".
> 
> Hi Michal,
> 
> yes, I've read the whole thread.

ok.

> 
>> I have attached the latest version I have and I am not going to change
>> it to follow "new" u-boot network driver style
> 
> Hm, but it is possible ...

sure, it is software almost everything is possible.

> 
>> because  I would like to keep ppc dcr support.
> 
> Even this was the main work I've done last week. I've split the driver
> code into differnt sub modules (xilinx_ll_temac_fifo and
> xilinx_ll_temac_sdram), introduce some call back functions into the sub
> modules to harmonize the main driver code and divide the sdma code into
> the two different bus access methodes: direct 32 bit memory access
> (Microblaze) and indirect access via DCR (Xilinx PPC4xx).
> 
> Please, give me some time to finish the refactoring. I'll release all
> results as fast as possible her in U-Boot list.

ok. Look forward for patches.

> 
> But, one issue was uncleare to me. Some functions, for example
> xps_ll_temac_hostif_set(), were prepared to set bit 10 of CTL register.
> But CTL bit 10 is not defined by any LL TEMAC documentation. Why did you
> done this?

I was in origin Yoshio Kashiwagi's driver. emac parameter should be possible to
remove entirely as below.

static unsigned int xps_ll_temac_hostif_get(struct eth_device *dev,
			int phy_addr, int reg_addr)
{
	struct temac_reg *regs = (struct temac_reg *)dev->iobase;

	out_be32(&regs->lsw, (phy_addr << 5) | reg_addr);
	out_be32(&regs->ctl, MIIMAI);
	xps_ll_temac_check_status(regs, XTE_RSE_MIIM_RR_MASK);
	return in_be32(&regs->lsw);
}

Thanks,
Michal