Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 03:55 PM, Paul Burton wrote:
> On Thu, May 26, 2016 at 02:05:07PM +0200, Marek Vasut wrote:
>>> Interesting :) May I ask which platform/SoC you're working with? Have
>>> you seen the (unfortunately currently out of tree) port we did for the
>>> JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small
>>> enough size well...
>>
>> I bought the CI20, yeah. I started with that tree and am now getting it
>> into mainline shape. I actually managed to optimize the SPL, so it does
>> not overflow. One of the things which helped a lot was removing slop[1]
>> from the structures and using appropriate data types in them (like don't
>> use u32 to store an 8bit number). I also tinified the MMC stack, so that
>> it won't pull in too much useless code.
>>
>> At this point, I have a u-boot running from SD card and the SD driver in
>> full U-Boot is already probing from DT and using DM. I sent Daniel the
>> updated patches, but they are still work in progress. Nonetheless, if
>> you want me forward that to you as well, let me know.
>>
>> I plan to get this CI20 submitted at some point this or next week, so it
>> can land in 2016.07 (I hope!) and then debian and fedora-mips (and other
>> distros) can pick it up and package proper. That's how I see it.
> 
> That sounds great :) I'd be interested in seeing them but probably won't
> have any time to look at them for a few weeks anyway (busy with other
> work things & then getting married & going off on honeymoon!). So
> hopefully by the time I'd get round to having a look they'll be in
> mainline :)

Congratulations ! I sent you a separate mail and I'm going to submit
some of the stuff now too.

>> btw. what is the linux support status of the ci20 ? Is anyone working
>> on that? I saw the DTS in Linux, but it's pretty bare-bones.
> 
> Yeah the support in mainline Linux is a bit basic at the moment. The
> biggest holdup is pinctrl support which I need to do some work on - the
> DT bindings are more complicated than they need to be, and the driver
> should probably be made more generic to cover other Ingenic SoCs before
> it goes into mainline.

I wonder if I should drill into that now ;-)

>> btw2. can I get the ci40 anywhere or is that just unobtainium board ?
> 
> I'll email you separately about that.
> 
> Thanks,
> Paul
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Simon Glass]

Hi Marek,

On 26 May 2016 at 11:35, Marek Vasut  wrote:
> On 05/26/2016 07:07 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 26 May 2016 at 10:53, Marek Vasut  wrote:
>>> On 05/26/2016 06:48 PM, Simon Glass wrote:
 Hi Marek,

 On 26 May 2016 at 10:47, Marek Vasut  wrote:
> On 05/26/2016 06:44 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 26 May 2016 at 10:34, Marek Vasut  wrote:
>>> On 05/26/2016 03:29 PM, Simon Glass wrote:
 Hi Marek,

 On 25 May 2016 at 16:35, Marek Vasut  wrote:
> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:


 Am 25.05.2016 um 02:19 schrieb Marek Vasut:
> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR 
> register
> in order to work at all. Add this special case handling into the 
> driver.
>
> Signed-off-by: Marek Vasut 
> Cc: Tom Rini 
> Cc: Simon Glass 
> Cc: Daniel Schwierzeck 
> Cc: Paul Burton 
> ---
>  drivers/serial/ns16550.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 30ba0aa..1323881 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  #endif
>
> +#ifdef CONFIG_ARCH_JZ47XX
> +#undef UART_FCRVAL
> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit 
> be set */
> +#define UART_FCR_UME  0x10
> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
> +   UART_FCR_TXSR | UART_FCR_UME)
> +#endif

 I think this could be added as DT property
>>>
>>> Not for SPL, which has 14 kiB size limit and it is itching to 
>>> overflow.
>>> I am literally counting bytes in the SPL and removing slop from
>>> structures to make it fit, just barely. With the USB loader, I can
>>> brutalize the SPL into having extremely rudimentary UART support now
>>> (like printch() being the most advanced output mechanism, but you 
>>> can
>>> only use it three times, otherwise the code won't fit and the board 
>>> is
>>> eaten by demons) and this is where this patch comes into play.
>>>
>>> So yes, for full u-boot, this _should_ be part of DT. For SPL, 
>>> please apply.
>>>
>>
>> ok, but wouldn't it be better to introduce an option like
>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>
> I was undecided between this (like the IER) and adding new ifdef (like
> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
> repugnant for sure.
>
> +
>  #ifndef CONFIG_SYS_NS16550_IER
>  #define CONFIG_SYS_NS16550_IER  0x00
>  #endif /* CONFIG_SYS_NS16550_IER */
>

 That way seems better to me. You should be able to add your UME flag
 as a Kconfig for this driver, in drivers/serial/Kconfig, which
 defaults to 0. It would be good to keep out board-specific stuff from
 this file, as you did with OMAP1510.
>>>
>>> I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
>>> via Kconfig. The extra bits should be set via DT props
>>> unless there is some really good reason why that cannot be done
>>> (like size limitation in SPL or DT not available).
>>>
>>> I tried these approaches:
>>> 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
>>>set specific byte value. This makes it hard to figure out
>>>which bits are set in the FCR just by looking at the value.
>>> 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
>>>use that in the driver to add the extra UME bit.
>>> 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
>>>using the macros from ns16550.h . This makes it obvious
>>>which bits are set.
>>>
>>> I am undecided between 2 and 3, but inclined to go with 3.
>>> What do you think ?
>>
>> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
>> which defaults to 0, but in your case is 

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 07:07 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 26 May 2016 at 10:53, Marek Vasut  wrote:
>> On 05/26/2016 06:48 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 26 May 2016 at 10:47, Marek Vasut  wrote:
 On 05/26/2016 06:44 PM, Simon Glass wrote:
> Hi Marek,
>
> On 26 May 2016 at 10:34, Marek Vasut  wrote:
>> On 05/26/2016 03:29 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 25 May 2016 at 16:35, Marek Vasut  wrote:
 On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>
>
> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>>
>>>
>>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
 The Ingenic JZ47xx requires special bit (UART_EN) set in FCR 
 register
 in order to work at all. Add this special case handling into the 
 driver.

 Signed-off-by: Marek Vasut 
 Cc: Tom Rini 
 Cc: Simon Glass 
 Cc: Daniel Schwierzeck 
 Cc: Paul Burton 
 ---
  drivers/serial/ns16550.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
 index 30ba0aa..1323881 100644
 --- a/drivers/serial/ns16550.c
 +++ b/drivers/serial/ns16550.c
 @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
  #endif
  #endif

 +#ifdef CONFIG_ARCH_JZ47XX
 +#undef UART_FCRVAL
 +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be 
 set */
 +#define UART_FCR_UME  0x10
 +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
 +   UART_FCR_TXSR | UART_FCR_UME)
 +#endif
>>>
>>> I think this could be added as DT property
>>
>> Not for SPL, which has 14 kiB size limit and it is itching to 
>> overflow.
>> I am literally counting bytes in the SPL and removing slop from
>> structures to make it fit, just barely. With the USB loader, I can
>> brutalize the SPL into having extremely rudimentary UART support now
>> (like printch() being the most advanced output mechanism, but you can
>> only use it three times, otherwise the code won't fit and the board 
>> is
>> eaten by demons) and this is where this patch comes into play.
>>
>> So yes, for full u-boot, this _should_ be part of DT. For SPL, 
>> please apply.
>>
>
> ok, but wouldn't it be better to introduce an option like
> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)

 I was undecided between this (like the IER) and adding new ifdef (like
 SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
 repugnant for sure.

 +
  #ifndef CONFIG_SYS_NS16550_IER
  #define CONFIG_SYS_NS16550_IER  0x00
  #endif /* CONFIG_SYS_NS16550_IER */

>>>
>>> That way seems better to me. You should be able to add your UME flag
>>> as a Kconfig for this driver, in drivers/serial/Kconfig, which
>>> defaults to 0. It would be good to keep out board-specific stuff from
>>> this file, as you did with OMAP1510.
>>
>> I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
>> via Kconfig. The extra bits should be set via DT props
>> unless there is some really good reason why that cannot be done
>> (like size limitation in SPL or DT not available).
>>
>> I tried these approaches:
>> 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
>>set specific byte value. This makes it hard to figure out
>>which bits are set in the FCR just by looking at the value.
>> 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
>>use that in the driver to add the extra UME bit.
>> 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
>>using the macros from ns16550.h . This makes it obvious
>>which bits are set.
>>
>> I am undecided between 2 and 3, but inclined to go with 3.
>> What do you think ?
>
> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
> which defaults to 0, but in your case is 0x10. You can OR it into the
> value.

 That's quite close to 1) , but then what if someone comes and wants to
 remove some bits from the FCR instead of adding some?
>>>
>>> Seems 

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Simon Glass]

Hi Marek,

On 26 May 2016 at 10:53, Marek Vasut  wrote:
> On 05/26/2016 06:48 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 26 May 2016 at 10:47, Marek Vasut  wrote:
>>> On 05/26/2016 06:44 PM, Simon Glass wrote:
 Hi Marek,

 On 26 May 2016 at 10:34, Marek Vasut  wrote:
> On 05/26/2016 03:29 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 25 May 2016 at 16:35, Marek Vasut  wrote:
>>> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:


 Am 26.05.2016 um 00:21 schrieb Marek Vasut:
> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR 
>>> register
>>> in order to work at all. Add this special case handling into the 
>>> driver.
>>>
>>> Signed-off-by: Marek Vasut 
>>> Cc: Tom Rini 
>>> Cc: Simon Glass 
>>> Cc: Daniel Schwierzeck 
>>> Cc: Paul Burton 
>>> ---
>>>  drivers/serial/ns16550.c | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> index 30ba0aa..1323881 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>>  #endif
>>>  #endif
>>>
>>> +#ifdef CONFIG_ARCH_JZ47XX
>>> +#undef UART_FCRVAL
>>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be 
>>> set */
>>> +#define UART_FCR_UME  0x10
>>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
>>> +   UART_FCR_TXSR | UART_FCR_UME)
>>> +#endif
>>
>> I think this could be added as DT property
>
> Not for SPL, which has 14 kiB size limit and it is itching to 
> overflow.
> I am literally counting bytes in the SPL and removing slop from
> structures to make it fit, just barely. With the USB loader, I can
> brutalize the SPL into having extremely rudimentary UART support now
> (like printch() being the most advanced output mechanism, but you can
> only use it three times, otherwise the code won't fit and the board is
> eaten by demons) and this is where this patch comes into play.
>
> So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
> apply.
>

 ok, but wouldn't it be better to introduce an option like
 CONFIG_SYS_NS16550_UME instead of using the SoC-specific
 CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>>>
>>> I was undecided between this (like the IER) and adding new ifdef (like
>>> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
>>> repugnant for sure.
>>>
>>> +
>>>  #ifndef CONFIG_SYS_NS16550_IER
>>>  #define CONFIG_SYS_NS16550_IER  0x00
>>>  #endif /* CONFIG_SYS_NS16550_IER */
>>>
>>
>> That way seems better to me. You should be able to add your UME flag
>> as a Kconfig for this driver, in drivers/serial/Kconfig, which
>> defaults to 0. It would be good to keep out board-specific stuff from
>> this file, as you did with OMAP1510.
>
> I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
> via Kconfig. The extra bits should be set via DT props
> unless there is some really good reason why that cannot be done
> (like size limitation in SPL or DT not available).
>
> I tried these approaches:
> 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
>set specific byte value. This makes it hard to figure out
>which bits are set in the FCR just by looking at the value.
> 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
>use that in the driver to add the extra UME bit.
> 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
>using the macros from ns16550.h . This makes it obvious
>which bits are set.
>
> I am undecided between 2 and 3, but inclined to go with 3.
> What do you think ?

 How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
 which defaults to 0, but in your case is 0x10. You can OR it into the
 value.
>>>
>>> That's quite close to 1) , but then what if someone comes and wants to
>>> remove some bits from the FCR instead of adding some?
>>
>> Seems like speculative complexity to me - let's worry about it when it 
>> happens!
>
> Heh, ok. But if we add Kconfig stuff, we can as well go with option 2) ,
> which would only 

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 06:48 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 26 May 2016 at 10:47, Marek Vasut  wrote:
>> On 05/26/2016 06:44 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 26 May 2016 at 10:34, Marek Vasut  wrote:
 On 05/26/2016 03:29 PM, Simon Glass wrote:
> Hi Marek,
>
> On 25 May 2016 at 16:35, Marek Vasut  wrote:
>> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>>
>>>
>>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
 On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>
>
> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>> in order to work at all. Add this special case handling into the 
>> driver.
>>
>> Signed-off-by: Marek Vasut 
>> Cc: Tom Rini 
>> Cc: Simon Glass 
>> Cc: Daniel Schwierzeck 
>> Cc: Paul Burton 
>> ---
>>  drivers/serial/ns16550.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 30ba0aa..1323881 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #endif
>>  #endif
>>
>> +#ifdef CONFIG_ARCH_JZ47XX
>> +#undef UART_FCRVAL
>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be 
>> set */
>> +#define UART_FCR_UME  0x10
>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
>> +   UART_FCR_TXSR | UART_FCR_UME)
>> +#endif
>
> I think this could be added as DT property

 Not for SPL, which has 14 kiB size limit and it is itching to overflow.
 I am literally counting bytes in the SPL and removing slop from
 structures to make it fit, just barely. With the USB loader, I can
 brutalize the SPL into having extremely rudimentary UART support now
 (like printch() being the most advanced output mechanism, but you can
 only use it three times, otherwise the code won't fit and the board is
 eaten by demons) and this is where this patch comes into play.

 So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
 apply.

>>>
>>> ok, but wouldn't it be better to introduce an option like
>>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>>
>> I was undecided between this (like the IER) and adding new ifdef (like
>> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
>> repugnant for sure.
>>
>> +
>>  #ifndef CONFIG_SYS_NS16550_IER
>>  #define CONFIG_SYS_NS16550_IER  0x00
>>  #endif /* CONFIG_SYS_NS16550_IER */
>>
>
> That way seems better to me. You should be able to add your UME flag
> as a Kconfig for this driver, in drivers/serial/Kconfig, which
> defaults to 0. It would be good to keep out board-specific stuff from
> this file, as you did with OMAP1510.

 I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
 via Kconfig. The extra bits should be set via DT props
 unless there is some really good reason why that cannot be done
 (like size limitation in SPL or DT not available).

 I tried these approaches:
 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
set specific byte value. This makes it hard to figure out
which bits are set in the FCR just by looking at the value.
 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
use that in the driver to add the extra UME bit.
 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
using the macros from ns16550.h . This makes it obvious
which bits are set.

 I am undecided between 2 and 3, but inclined to go with 3.
 What do you think ?
>>>
>>> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
>>> which defaults to 0, but in your case is 0x10. You can OR it into the
>>> value.
>>
>> That's quite close to 1) , but then what if someone comes and wants to
>> remove some bits from the FCR instead of adding some?
> 
> Seems like speculative complexity to me - let's worry about it when it 
> happens!

Maybe this is not even worth the effort and I should just ignore serial
support in SPL and only go with serial support in full u-boot, where I
can configure the FCR from DT. Then we avoid all this complex stuff.


-- 
Best regards,
Marek Vasut
___
U-Boot 

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 06:48 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 26 May 2016 at 10:47, Marek Vasut  wrote:
>> On 05/26/2016 06:44 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 26 May 2016 at 10:34, Marek Vasut  wrote:
 On 05/26/2016 03:29 PM, Simon Glass wrote:
> Hi Marek,
>
> On 25 May 2016 at 16:35, Marek Vasut  wrote:
>> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>>
>>>
>>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
 On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>
>
> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>> in order to work at all. Add this special case handling into the 
>> driver.
>>
>> Signed-off-by: Marek Vasut 
>> Cc: Tom Rini 
>> Cc: Simon Glass 
>> Cc: Daniel Schwierzeck 
>> Cc: Paul Burton 
>> ---
>>  drivers/serial/ns16550.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 30ba0aa..1323881 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #endif
>>  #endif
>>
>> +#ifdef CONFIG_ARCH_JZ47XX
>> +#undef UART_FCRVAL
>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be 
>> set */
>> +#define UART_FCR_UME  0x10
>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
>> +   UART_FCR_TXSR | UART_FCR_UME)
>> +#endif
>
> I think this could be added as DT property

 Not for SPL, which has 14 kiB size limit and it is itching to overflow.
 I am literally counting bytes in the SPL and removing slop from
 structures to make it fit, just barely. With the USB loader, I can
 brutalize the SPL into having extremely rudimentary UART support now
 (like printch() being the most advanced output mechanism, but you can
 only use it three times, otherwise the code won't fit and the board is
 eaten by demons) and this is where this patch comes into play.

 So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
 apply.

>>>
>>> ok, but wouldn't it be better to introduce an option like
>>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>>
>> I was undecided between this (like the IER) and adding new ifdef (like
>> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
>> repugnant for sure.
>>
>> +
>>  #ifndef CONFIG_SYS_NS16550_IER
>>  #define CONFIG_SYS_NS16550_IER  0x00
>>  #endif /* CONFIG_SYS_NS16550_IER */
>>
>
> That way seems better to me. You should be able to add your UME flag
> as a Kconfig for this driver, in drivers/serial/Kconfig, which
> defaults to 0. It would be good to keep out board-specific stuff from
> this file, as you did with OMAP1510.

 I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
 via Kconfig. The extra bits should be set via DT props
 unless there is some really good reason why that cannot be done
 (like size limitation in SPL or DT not available).

 I tried these approaches:
 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
set specific byte value. This makes it hard to figure out
which bits are set in the FCR just by looking at the value.
 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
use that in the driver to add the extra UME bit.
 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
using the macros from ns16550.h . This makes it obvious
which bits are set.

 I am undecided between 2 and 3, but inclined to go with 3.
 What do you think ?
>>>
>>> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
>>> which defaults to 0, but in your case is 0x10. You can OR it into the
>>> value.
>>
>> That's quite close to 1) , but then what if someone comes and wants to
>> remove some bits from the FCR instead of adding some?
> 
> Seems like speculative complexity to me - let's worry about it when it 
> happens!

Heh, ok. But if we add Kconfig stuff, we can as well go with option 2) ,
which would only show up if JZ47XX is selected. Since this bits is
specific to this lineup of SoCs , that makes sense in my head. What do
you think ?


-- 
Best regards,
Marek Vasut

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Simon Glass]

Hi Marek,

On 26 May 2016 at 10:47, Marek Vasut  wrote:
> On 05/26/2016 06:44 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 26 May 2016 at 10:34, Marek Vasut  wrote:
>>> On 05/26/2016 03:29 PM, Simon Glass wrote:
 Hi Marek,

 On 25 May 2016 at 16:35, Marek Vasut  wrote:
> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:


 Am 25.05.2016 um 02:19 schrieb Marek Vasut:
> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
> in order to work at all. Add this special case handling into the 
> driver.
>
> Signed-off-by: Marek Vasut 
> Cc: Tom Rini 
> Cc: Simon Glass 
> Cc: Daniel Schwierzeck 
> Cc: Paul Burton 
> ---
>  drivers/serial/ns16550.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 30ba0aa..1323881 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  #endif
>
> +#ifdef CONFIG_ARCH_JZ47XX
> +#undef UART_FCRVAL
> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be 
> set */
> +#define UART_FCR_UME  0x10
> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
> +   UART_FCR_TXSR | UART_FCR_UME)
> +#endif

 I think this could be added as DT property
>>>
>>> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
>>> I am literally counting bytes in the SPL and removing slop from
>>> structures to make it fit, just barely. With the USB loader, I can
>>> brutalize the SPL into having extremely rudimentary UART support now
>>> (like printch() being the most advanced output mechanism, but you can
>>> only use it three times, otherwise the code won't fit and the board is
>>> eaten by demons) and this is where this patch comes into play.
>>>
>>> So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
>>> apply.
>>>
>>
>> ok, but wouldn't it be better to introduce an option like
>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>
> I was undecided between this (like the IER) and adding new ifdef (like
> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
> repugnant for sure.
>
> +
>  #ifndef CONFIG_SYS_NS16550_IER
>  #define CONFIG_SYS_NS16550_IER  0x00
>  #endif /* CONFIG_SYS_NS16550_IER */
>

 That way seems better to me. You should be able to add your UME flag
 as a Kconfig for this driver, in drivers/serial/Kconfig, which
 defaults to 0. It would be good to keep out board-specific stuff from
 this file, as you did with OMAP1510.
>>>
>>> I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
>>> via Kconfig. The extra bits should be set via DT props
>>> unless there is some really good reason why that cannot be done
>>> (like size limitation in SPL or DT not available).
>>>
>>> I tried these approaches:
>>> 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
>>>set specific byte value. This makes it hard to figure out
>>>which bits are set in the FCR just by looking at the value.
>>> 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
>>>use that in the driver to add the extra UME bit.
>>> 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
>>>using the macros from ns16550.h . This makes it obvious
>>>which bits are set.
>>>
>>> I am undecided between 2 and 3, but inclined to go with 3.
>>> What do you think ?
>>
>> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
>> which defaults to 0, but in your case is 0x10. You can OR it into the
>> value.
>
> That's quite close to 1) , but then what if someone comes and wants to
> remove some bits from the FCR instead of adding some?

Seems like speculative complexity to me - let's worry about it when it happens!

- Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 06:44 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 26 May 2016 at 10:34, Marek Vasut  wrote:
>> On 05/26/2016 03:29 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 25 May 2016 at 16:35, Marek Vasut  wrote:
 On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>
>
> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>>
>>>
>>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
 The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
 in order to work at all. Add this special case handling into the 
 driver.

 Signed-off-by: Marek Vasut 
 Cc: Tom Rini 
 Cc: Simon Glass 
 Cc: Daniel Schwierzeck 
 Cc: Paul Burton 
 ---
  drivers/serial/ns16550.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
 index 30ba0aa..1323881 100644
 --- a/drivers/serial/ns16550.c
 +++ b/drivers/serial/ns16550.c
 @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
  #endif
  #endif

 +#ifdef CONFIG_ARCH_JZ47XX
 +#undef UART_FCRVAL
 +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set 
 */
 +#define UART_FCR_UME  0x10
 +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
 +   UART_FCR_TXSR | UART_FCR_UME)
 +#endif
>>>
>>> I think this could be added as DT property
>>
>> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
>> I am literally counting bytes in the SPL and removing slop from
>> structures to make it fit, just barely. With the USB loader, I can
>> brutalize the SPL into having extremely rudimentary UART support now
>> (like printch() being the most advanced output mechanism, but you can
>> only use it three times, otherwise the code won't fit and the board is
>> eaten by demons) and this is where this patch comes into play.
>>
>> So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
>> apply.
>>
>
> ok, but wouldn't it be better to introduce an option like
> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)

 I was undecided between this (like the IER) and adding new ifdef (like
 SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
 repugnant for sure.

 +
  #ifndef CONFIG_SYS_NS16550_IER
  #define CONFIG_SYS_NS16550_IER  0x00
  #endif /* CONFIG_SYS_NS16550_IER */

>>>
>>> That way seems better to me. You should be able to add your UME flag
>>> as a Kconfig for this driver, in drivers/serial/Kconfig, which
>>> defaults to 0. It would be good to keep out board-specific stuff from
>>> this file, as you did with OMAP1510.
>>
>> I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
>> via Kconfig. The extra bits should be set via DT props
>> unless there is some really good reason why that cannot be done
>> (like size limitation in SPL or DT not available).
>>
>> I tried these approaches:
>> 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
>>set specific byte value. This makes it hard to figure out
>>which bits are set in the FCR just by looking at the value.
>> 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
>>use that in the driver to add the extra UME bit.
>> 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
>>using the macros from ns16550.h . This makes it obvious
>>which bits are set.
>>
>> I am undecided between 2 and 3, but inclined to go with 3.
>> What do you think ?
> 
> How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
> which defaults to 0, but in your case is 0x10. You can OR it into the
> value.

That's quite close to 1) , but then what if someone comes and wants to
remove some bits from the FCR instead of adding some?

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Simon Glass]

Hi Marek,

On 26 May 2016 at 10:34, Marek Vasut  wrote:
> On 05/26/2016 03:29 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 25 May 2016 at 16:35, Marek Vasut  wrote:
>>> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:


 Am 26.05.2016 um 00:21 schrieb Marek Vasut:
> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>>> in order to work at all. Add this special case handling into the driver.
>>>
>>> Signed-off-by: Marek Vasut 
>>> Cc: Tom Rini 
>>> Cc: Simon Glass 
>>> Cc: Daniel Schwierzeck 
>>> Cc: Paul Burton 
>>> ---
>>>  drivers/serial/ns16550.c | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> index 30ba0aa..1323881 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>>  #endif
>>>  #endif
>>>
>>> +#ifdef CONFIG_ARCH_JZ47XX
>>> +#undef UART_FCRVAL
>>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set 
>>> */
>>> +#define UART_FCR_UME  0x10
>>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
>>> +   UART_FCR_TXSR | UART_FCR_UME)
>>> +#endif
>>
>> I think this could be added as DT property
>
> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
> I am literally counting bytes in the SPL and removing slop from
> structures to make it fit, just barely. With the USB loader, I can
> brutalize the SPL into having extremely rudimentary UART support now
> (like printch() being the most advanced output mechanism, but you can
> only use it three times, otherwise the code won't fit and the board is
> eaten by demons) and this is where this patch comes into play.
>
> So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
> apply.
>

 ok, but wouldn't it be better to introduce an option like
 CONFIG_SYS_NS16550_UME instead of using the SoC-specific
 CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>>>
>>> I was undecided between this (like the IER) and adding new ifdef (like
>>> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
>>> repugnant for sure.
>>>
>>> +
>>>  #ifndef CONFIG_SYS_NS16550_IER
>>>  #define CONFIG_SYS_NS16550_IER  0x00
>>>  #endif /* CONFIG_SYS_NS16550_IER */
>>>
>>
>> That way seems better to me. You should be able to add your UME flag
>> as a Kconfig for this driver, in drivers/serial/Kconfig, which
>> defaults to 0. It would be good to keep out board-specific stuff from
>> this file, as you did with OMAP1510.
>
> I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
> via Kconfig. The extra bits should be set via DT props
> unless there is some really good reason why that cannot be done
> (like size limitation in SPL or DT not available).
>
> I tried these approaches:
> 1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
>set specific byte value. This makes it hard to figure out
>which bits are set in the FCR just by looking at the value.
> 2) Add Kconfig option selected if ARCH_JZ47XX is selected and
>use that in the driver to add the extra UME bit.
> 3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
>using the macros from ns16550.h . This makes it obvious
>which bits are set.
>
> I am undecided between 2 and 3, but inclined to go with 3.
> What do you think ?

How about making your feature an option, like CONFIG_SYS_NS16550_UEN,
which defaults to 0, but in your case is 0x10. You can OR it into the
value.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 03:29 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 25 May 2016 at 16:35, Marek Vasut  wrote:
>> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>>
>>>
>>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
 On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>
>
> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>> in order to work at all. Add this special case handling into the driver.
>>
>> Signed-off-by: Marek Vasut 
>> Cc: Tom Rini 
>> Cc: Simon Glass 
>> Cc: Daniel Schwierzeck 
>> Cc: Paul Burton 
>> ---
>>  drivers/serial/ns16550.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 30ba0aa..1323881 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #endif
>>  #endif
>>
>> +#ifdef CONFIG_ARCH_JZ47XX
>> +#undef UART_FCRVAL
>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
>> +#define UART_FCR_UME  0x10
>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
>> +   UART_FCR_TXSR | UART_FCR_UME)
>> +#endif
>
> I think this could be added as DT property

 Not for SPL, which has 14 kiB size limit and it is itching to overflow.
 I am literally counting bytes in the SPL and removing slop from
 structures to make it fit, just barely. With the USB loader, I can
 brutalize the SPL into having extremely rudimentary UART support now
 (like printch() being the most advanced output mechanism, but you can
 only use it three times, otherwise the code won't fit and the board is
 eaten by demons) and this is where this patch comes into play.

 So yes, for full u-boot, this _should_ be part of DT. For SPL, please 
 apply.

>>>
>>> ok, but wouldn't it be better to introduce an option like
>>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>>
>> I was undecided between this (like the IER) and adding new ifdef (like
>> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
>> repugnant for sure.
>>
>> +
>>  #ifndef CONFIG_SYS_NS16550_IER
>>  #define CONFIG_SYS_NS16550_IER  0x00
>>  #endif /* CONFIG_SYS_NS16550_IER */
>>
> 
> That way seems better to me. You should be able to add your UME flag
> as a Kconfig for this driver, in drivers/serial/Kconfig, which
> defaults to 0. It would be good to keep out board-specific stuff from
> this file, as you did with OMAP1510.

I'm not really sure I want to expose the CONFIG_SYS_NS16550_FCR override
via Kconfig. The extra bits should be set via DT props
unless there is some really good reason why that cannot be done
(like size limitation in SPL or DT not available).

I tried these approaches:
1) Add Kconfig option for CONFIG_SYS_NS16550_FCR , where you
   set specific byte value. This makes it hard to figure out
   which bits are set in the FCR just by looking at the value.
2) Add Kconfig option selected if ARCH_JZ47XX is selected and
   use that in the driver to add the extra UME bit.
3) Define CONFIG_SYS_NS16550_FCR in include/configs/board.h
   using the macros from ns16550.h . This makes it obvious
   which bits are set.

I am undecided between 2 and 3, but inclined to go with 3.
What do you think ?

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Paul Burton]

On Thu, May 26, 2016 at 02:05:07PM +0200, Marek Vasut wrote:
> > Interesting :) May I ask which platform/SoC you're working with? Have
> > you seen the (unfortunately currently out of tree) port we did for the
> > JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small
> > enough size well...
> 
> I bought the CI20, yeah. I started with that tree and am now getting it
> into mainline shape. I actually managed to optimize the SPL, so it does
> not overflow. One of the things which helped a lot was removing slop[1]
> from the structures and using appropriate data types in them (like don't
> use u32 to store an 8bit number). I also tinified the MMC stack, so that
> it won't pull in too much useless code.
> 
> At this point, I have a u-boot running from SD card and the SD driver in
> full U-Boot is already probing from DT and using DM. I sent Daniel the
> updated patches, but they are still work in progress. Nonetheless, if
> you want me forward that to you as well, let me know.
> 
> I plan to get this CI20 submitted at some point this or next week, so it
> can land in 2016.07 (I hope!) and then debian and fedora-mips (and other
> distros) can pick it up and package proper. That's how I see it.

That sounds great :) I'd be interested in seeing them but probably won't
have any time to look at them for a few weeks anyway (busy with other
work things & then getting married & going off on honeymoon!). So
hopefully by the time I'd get round to having a look they'll be in
mainline :)

> btw. what is the linux support status of the ci20 ? Is anyone working
> on that? I saw the DTS in Linux, but it's pretty bare-bones.

Yeah the support in mainline Linux is a bit basic at the moment. The
biggest holdup is pinctrl support which I need to do some work on - the
DT bindings are more complicated than they need to be, and the driver
should probably be made more generic to cover other Ingenic SoCs before
it goes into mainline.

> btw2. can I get the ci40 anywhere or is that just unobtainium board ?

I'll email you separately about that.

Thanks,
Paul
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Simon Glass]

Hi Marek,

On 25 May 2016 at 16:35, Marek Vasut  wrote:
> On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:


 Am 25.05.2016 um 02:19 schrieb Marek Vasut:
> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
> in order to work at all. Add this special case handling into the driver.
>
> Signed-off-by: Marek Vasut 
> Cc: Tom Rini 
> Cc: Simon Glass 
> Cc: Daniel Schwierzeck 
> Cc: Paul Burton 
> ---
>  drivers/serial/ns16550.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 30ba0aa..1323881 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  #endif
>
> +#ifdef CONFIG_ARCH_JZ47XX
> +#undef UART_FCRVAL
> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
> +#define UART_FCR_UME  0x10
> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
> +   UART_FCR_TXSR | UART_FCR_UME)
> +#endif

 I think this could be added as DT property
>>>
>>> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
>>> I am literally counting bytes in the SPL and removing slop from
>>> structures to make it fit, just barely. With the USB loader, I can
>>> brutalize the SPL into having extremely rudimentary UART support now
>>> (like printch() being the most advanced output mechanism, but you can
>>> only use it three times, otherwise the code won't fit and the board is
>>> eaten by demons) and this is where this patch comes into play.
>>>
>>> So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.
>>>
>>
>> ok, but wouldn't it be better to introduce an option like
>> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
>> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)
>
> I was undecided between this (like the IER) and adding new ifdef (like
> SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
> repugnant for sure.
>
> +
>  #ifndef CONFIG_SYS_NS16550_IER
>  #define CONFIG_SYS_NS16550_IER  0x00
>  #endif /* CONFIG_SYS_NS16550_IER */
>

That way seems better to me. You should be able to add your UME flag
as a Kconfig for this driver, in drivers/serial/Kconfig, which
defaults to 0. It would be good to keep out board-specific stuff from
this file, as you did with OMAP1510.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 01:46 PM, Paul Burton wrote:
> On Thu, May 26, 2016 at 12:21:23AM +0200, Marek Vasut wrote:
>> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
>> I am literally counting bytes in the SPL and removing slop from
>> structures to make it fit, just barely. With the USB loader, I can
>> brutalize the SPL into having extremely rudimentary UART support now
>> (like printch() being the most advanced output mechanism, but you can
>> only use it three times, otherwise the code won't fit and the board is
>> eaten by demons) and this is where this patch comes into play.
>>
>> So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.
> 
> Hi Marek,

Hi!

> Interesting :) May I ask which platform/SoC you're working with? Have
> you seen the (unfortunately currently out of tree) port we did for the
> JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small
> enough size well...

I bought the CI20, yeah. I started with that tree and am now getting it
into mainline shape. I actually managed to optimize the SPL, so it does
not overflow. One of the things which helped a lot was removing slop[1]
from the structures and using appropriate data types in them (like don't
use u32 to store an 8bit number). I also tinified the MMC stack, so that
it won't pull in too much useless code.

At this point, I have a u-boot running from SD card and the SD driver in
full U-Boot is already probing from DT and using DM. I sent Daniel the
updated patches, but they are still work in progress. Nonetheless, if
you want me forward that to you as well, let me know.

I plan to get this CI20 submitted at some point this or next week, so it
can land in 2016.07 (I hope!) and then debian and fedora-mips (and other
distros) can pick it up and package proper. That's how I see it.

btw. what is the linux support status of the ci20 ? Is anyone working
on that? I saw the DTS in Linux, but it's pretty bare-bones.

btw2. can I get the ci40 anywhere or is that just unobtainium board ?

> https://github.com/MIPS/CI20_u-boot
> 
> Thanks,
> Paul
> 

[1] http://www.catb.org/esr/structure-packing/

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Paul Burton]

On Thu, May 26, 2016 at 12:21:23AM +0200, Marek Vasut wrote:
> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
> I am literally counting bytes in the SPL and removing slop from
> structures to make it fit, just barely. With the USB loader, I can
> brutalize the SPL into having extremely rudimentary UART support now
> (like printch() being the most advanced output mechanism, but you can
> only use it three times, otherwise the code won't fit and the board is
> eaten by demons) and this is where this patch comes into play.
> 
> So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.

Hi Marek,

Interesting :) May I ask which platform/SoC you're working with? Have
you seen the (unfortunately currently out of tree) port we did for the
JZ4780-based Ci20? I recall the pain of squeezing SPL down to a small
enough size well...

https://github.com/MIPS/CI20_u-boot

Thanks,
Paul
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 12:31 AM, Daniel Schwierzeck wrote:
> 
> 
> Am 26.05.2016 um 00:21 schrieb Marek Vasut:
>> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>>
>>>
>>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
 The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
 in order to work at all. Add this special case handling into the driver.

 Signed-off-by: Marek Vasut 
 Cc: Tom Rini 
 Cc: Simon Glass 
 Cc: Daniel Schwierzeck 
 Cc: Paul Burton 
 ---
  drivers/serial/ns16550.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
 index 30ba0aa..1323881 100644
 --- a/drivers/serial/ns16550.c
 +++ b/drivers/serial/ns16550.c
 @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
  #endif
  #endif
  
 +#ifdef CONFIG_ARCH_JZ47XX
 +#undef UART_FCRVAL
 +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
 +#define UART_FCR_UME  0x10
 +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |   \
 +   UART_FCR_TXSR | UART_FCR_UME)
 +#endif
>>>
>>> I think this could be added as DT property
>>
>> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
>> I am literally counting bytes in the SPL and removing slop from
>> structures to make it fit, just barely. With the USB loader, I can
>> brutalize the SPL into having extremely rudimentary UART support now
>> (like printch() being the most advanced output mechanism, but you can
>> only use it three times, otherwise the code won't fit and the board is
>> eaten by demons) and this is where this patch comes into play.
>>
>> So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.
>>
> 
> ok, but wouldn't it be better to introduce an option like
> CONFIG_SYS_NS16550_UME instead of using the SoC-specific
> CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)

I was undecided between this (like the IER) and adding new ifdef (like
SOC_KEYSTONE). Whichever way is fine with me. Yeah, the driver is
repugnant for sure.

 +
  #ifndef CONFIG_SYS_NS16550_IER
  #define CONFIG_SYS_NS16550_IER  0x00
  #endif /* CONFIG_SYS_NS16550_IER */

>>>
>>
>>
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Daniel Schwierzeck]

Am 26.05.2016 um 00:21 schrieb Marek Vasut:
> On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
>>
>>
>> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>>> in order to work at all. Add this special case handling into the driver.
>>>
>>> Signed-off-by: Marek Vasut 
>>> Cc: Tom Rini 
>>> Cc: Simon Glass 
>>> Cc: Daniel Schwierzeck 
>>> Cc: Paul Burton 
>>> ---
>>>  drivers/serial/ns16550.c | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>>> index 30ba0aa..1323881 100644
>>> --- a/drivers/serial/ns16550.c
>>> +++ b/drivers/serial/ns16550.c
>>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>>  #endif
>>>  #endif
>>>  
>>> +#ifdef CONFIG_ARCH_JZ47XX
>>> +#undef UART_FCRVAL
>>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
>>> +#define UART_FCR_UME   0x10
>>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |\
>>> +UART_FCR_TXSR | UART_FCR_UME)
>>> +#endif
>>
>> I think this could be added as DT property
> 
> Not for SPL, which has 14 kiB size limit and it is itching to overflow.
> I am literally counting bytes in the SPL and removing slop from
> structures to make it fit, just barely. With the USB loader, I can
> brutalize the SPL into having extremely rudimentary UART support now
> (like printch() being the most advanced output mechanism, but you can
> only use it three times, otherwise the code won't fit and the board is
> eaten by demons) and this is where this patch comes into play.
> 
> So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.
> 

ok, but wouldn't it be better to introduce an option like
CONFIG_SYS_NS16550_UME instead of using the SoC-specific
CONFIG_ARCH_JZ47XX. This driver is messed up enough ;)

>>> +
>>>  #ifndef CONFIG_SYS_NS16550_IER
>>>  #define CONFIG_SYS_NS16550_IER  0x00
>>>  #endif /* CONFIG_SYS_NS16550_IER */
>>>
>>
> 
> 

-- 
- Daniel



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

On 05/26/2016 12:17 AM, Daniel Schwierzeck wrote:
> 
> 
> Am 25.05.2016 um 02:19 schrieb Marek Vasut:
>> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
>> in order to work at all. Add this special case handling into the driver.
>>
>> Signed-off-by: Marek Vasut 
>> Cc: Tom Rini 
>> Cc: Simon Glass 
>> Cc: Daniel Schwierzeck 
>> Cc: Paul Burton 
>> ---
>>  drivers/serial/ns16550.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
>> index 30ba0aa..1323881 100644
>> --- a/drivers/serial/ns16550.c
>> +++ b/drivers/serial/ns16550.c
>> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>  #endif
>>  #endif
>>  
>> +#ifdef CONFIG_ARCH_JZ47XX
>> +#undef UART_FCRVAL
>> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
>> +#define UART_FCR_UME0x10
>> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR | \
>> + UART_FCR_TXSR | UART_FCR_UME)
>> +#endif
> 
> I think this could be added as DT property

Not for SPL, which has 14 kiB size limit and it is itching to overflow.
I am literally counting bytes in the SPL and removing slop from
structures to make it fit, just barely. With the USB loader, I can
brutalize the SPL into having extremely rudimentary UART support now
(like printch() being the most advanced output mechanism, but you can
only use it three times, otherwise the code won't fit and the board is
eaten by demons) and this is where this patch comes into play.

So yes, for full u-boot, this _should_ be part of DT. For SPL, please apply.

>> +
>>  #ifndef CONFIG_SYS_NS16550_IER
>>  #define CONFIG_SYS_NS16550_IER  0x00
>>  #endif /* CONFIG_SYS_NS16550_IER */
>>
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Daniel Schwierzeck]

Am 25.05.2016 um 02:19 schrieb Marek Vasut:
> The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
> in order to work at all. Add this special case handling into the driver.
> 
> Signed-off-by: Marek Vasut 
> Cc: Tom Rini 
> Cc: Simon Glass 
> Cc: Daniel Schwierzeck 
> Cc: Paul Burton 
> ---
>  drivers/serial/ns16550.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
> index 30ba0aa..1323881 100644
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
>  #endif
>  #endif
>  
> +#ifdef CONFIG_ARCH_JZ47XX
> +#undef UART_FCRVAL
> +/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
> +#define UART_FCR_UME 0x10
> +#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |  \
> +  UART_FCR_TXSR | UART_FCR_UME)
> +#endif

I think this could be added as DT property

> +
>  #ifndef CONFIG_SYS_NS16550_IER
>  #define CONFIG_SYS_NS16550_IER  0x00
>  #endif /* CONFIG_SYS_NS16550_IER */
> 

-- 
- Daniel



signature.asc
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] serial: 16550: Add JZ47xx support

[Marek Vasut]

The Ingenic JZ47xx requires special bit (UART_EN) set in FCR register
in order to work at all. Add this special case handling into the driver.

Signed-off-by: Marek Vasut 
Cc: Tom Rini 
Cc: Simon Glass 
Cc: Daniel Schwierzeck 
Cc: Paul Burton 
---
 drivers/serial/ns16550.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 30ba0aa..1323881 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -50,6 +50,14 @@ DECLARE_GLOBAL_DATA_PTR;
 #endif
 #endif
 
+#ifdef CONFIG_ARCH_JZ47XX
+#undef UART_FCRVAL
+/* Ingenic JZ47xx SoCs require that a 'UART Module Enable' bit be set */
+#define UART_FCR_UME   0x10
+#define UART_FCRVAL (UART_FCR_FIFO_EN | UART_FCR_RXSR |\
+UART_FCR_TXSR | UART_FCR_UME)
+#endif
+
 #ifndef CONFIG_SYS_NS16550_IER
 #define CONFIG_SYS_NS16550_IER  0x00
 #endif /* CONFIG_SYS_NS16550_IER */
-- 
2.7.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot